Project

General

Profile

Actions

Bug #8508

closed

[Data Manager] Tests assume you can't write to /

Added by Joshua Randall almost 9 years ago. Updated almost 9 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Tests
Target version:
Story points:
0.5

Description

it seems that there are a few datamanager tests that assume they won't be able to write to '/' and thus fail if files can in fact be written (e.g. TestPutAndGetBlocks_ErrorDuringGetCollectionsBadWriteTo expects to not be able to write to /badwritetofile and TestPutAndGetBlocks_ErrorDuringGetCollectionsBadHeapProfileFilename expects to not be able to write to /badheapprofile).

It would be better to ensure the failure by creating an explicit conflict. Perhaps the test could create a "bad" subdir in the test tmp directory and `chmod 000` it, then put the bad files under that directory.


Subtasks 1 (0 open1 closed)

Task #8518: Review GitHub PR #42ResolvedJoshua Randall02/23/2016Actions
Actions #1

Updated by Joshua Randall almost 9 years ago

  • Status changed from New to Feedback
  • Assigned To set to Joshua Randall
Actions #2

Updated by Joshua Randall almost 9 years ago

  • Assigned To deleted (Joshua Randall)
Actions #3

Updated by Brett Smith almost 9 years ago

  • Subject changed from datamanager tests assume you can't write to / to [Data Manager] Tests assume you can't write to /
Actions #4

Updated by Brett Smith almost 9 years ago

  • Target version set to 2016-03-16 sprint
Actions #5

Updated by Joshua Randall almost 9 years ago

  • Assigned To set to Joshua Randall
Actions #6

Updated by Brett Smith almost 9 years ago

  • Story points set to 0.5
Actions #7

Updated by Radhika Chippada almost 9 years ago

Reviewing branch wtsi-hgi-8508-datamanager-test-badpaths:

  • It appears that we can ensure the dir is not accessible by simply replacing "/badwritetofile" with "/nosuchdir/badwritetofile" or fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds()) rather than creating a tempdir and appending a string to it? Please consider this simplified approach instead.
  testOldBlocksNotDeletedOnDataManagerError(t, fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds()), "", true, true)
  • Also, please run go fmt

Thanks.

Actions #8

Updated by Joshua Randall almost 9 years ago

The simplified approach "/nosuchdir/badwritetofile" would probably work in most cases but it is trivial to break it by creating a "/nosuchdir". This realistically could occur accidentally, so I do not favor this option.

The slightly less simplified approach `fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds())` would almost definitely work in every conceivable scenario although it would still theoretically be possible to break it by creating a large number of /nosuchdir%v paths with future times. I agree this is extremely unlikely to ever be an issue unless someone is "attacking" the test intentionally.

The proposed approach (creating a temp dir that we "own" and then referring to a nonexistent path within it) should always work unless some concurrent process creates the bad path in our temporary directory after we create the temporary directory (i.e. an active attack against the test).

I went for the proposed approach because it is the safest thing I could think of. If you are concerned about the extra temporary directory creation, I'm happy to compromise on your timestamp based solution.

Actions #9

Updated by Radhika Chippada almost 9 years ago

The extra step of creating the tmp dir is not much more expensive to execute and so let's stick with your implemented solution. I will merge your branch shortly. Thanks.

Actions #10

Updated by Radhika Chippada almost 9 years ago

  • Status changed from Feedback to Resolved

Applied in changeset arvados|commit:c6df16d2af30e989bcfb04f6ef730cde658a9dc9.

Actions

Also available in: Atom PDF