| Message ID | 6a0119bae65b4b8daf8af135dfedc9ba@rwthex-w2-b.rwth-ad.de |
|---|---|
| State | Superseded |
| Headers | show |
On 09/11/2016 02:46 PM, Stefan Brüns wrote: > <path>/<fname> and <path>/./<fname> should reference the same file. > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh > +# Read 1MB from small file > +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL I think the same issue with $FPATH ending/not-ending in / applies here too, and for all commands in this patch. > @@ -482,6 +499,16 @@ function check_results() { > + # Check directory traversal > + grep -A6 "Test Case 13a " "$1" | \ > + egrep -q '1048576 bytes written|update journal' Why is "update journal" considered successful? Surely the "n bytes written" message is always printed irrespective of whether anything journal-related happened?
On Montag, 12. September 2016 12:44:08 CEST you wrote: > On 09/11/2016 02:46 PM, Stefan Brüns wrote: > > <path>/<fname> and <path>/./<fname> should reference the same file. > > > > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh > > > > +# Read 1MB from small file > > +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL > > I think the same issue with $FPATH ending/not-ending in / applies here > too, and for all commands in this patch. FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for hostfs, so this is correct. Specifically, for fat, we dont want any "/" in front of $FILE_foo. > > @@ -482,6 +499,16 @@ function check_results() { > > > > + # Check directory traversal > > + grep -A6 "Test Case 13a " "$1" | \ > > + egrep -q '1048576 bytes written|update journal' > > Why is "update journal" considered successful? Surely the "n bytes > written" message is always printed irrespective of whether anything > journal-related happened? Thats a question left to the author of Test Case 11, where the fragment was copied from. Ext4 unfortunately is quite verbose, it inserts "File system is consistent" and "update journal finished" lines in the output. I think these lines where better stripped from the log prior to any further parsing. Kind regards, Stefan
On 09/12/2016 04:04 PM, Stefan Bruens wrote: > On Montag, 12. September 2016 12:44:08 CEST you wrote: >> On 09/11/2016 02:46 PM, Stefan Brüns wrote: >>> <path>/<fname> and <path>/./<fname> should reference the same file. >>> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh >>> >>> +# Read 1MB from small file >>> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL >> >> I think the same issue with $FPATH ending/not-ending in / applies here >> too, and for all commands in this patch. > > FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for > hostfs, so this is correct. Specifically, for fat, we dont want any "/" in > front of $FILE_foo. I believe FPATH can be either "", "/" or "/foo/bar" here, due to the issue I just mentioned in the other email. >>> @@ -482,6 +499,16 @@ function check_results() { >>> >>> + # Check directory traversal >>> + grep -A6 "Test Case 13a " "$1" | \ >>> + egrep -q '1048576 bytes written|update journal' >> >> Why is "update journal" considered successful? Surely the "n bytes >> written" message is always printed irrespective of whether anything >> journal-related happened? > > Thats a question left to the author of Test Case 11, where the fragment was > copied from. I don't quite agree; you're adding the new test, so should make sure the validation code makes sense. > Ext4 unfortunately is quite verbose, it inserts "File system is consistent" > and "update journal finished" lines in the output. I think these lines where > better stripped from the log prior to any further parsing. ext4 may print some extra output, but that doesn't mean that any of it should be used in the validation. I think the simplest thing is to just ignore it in the validation code. Using egrep -q '1048576 bytes written' should do that just fine, and not get a false-positive if ext4 does say "update journal" without saying the required "1048576 bytes written".
On Dienstag, 13. September 2016 12:36:26 CEST you wrote: > On 09/12/2016 04:04 PM, Stefan Bruens wrote: > > On Montag, 12. September 2016 12:44:08 CEST you wrote: > >> On 09/11/2016 02:46 PM, Stefan Brüns wrote: > >>> <path>/<fname> and <path>/./<fname> should reference the same file. > >>> > >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh > >>> > >>> +# Read 1MB from small file > >>> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL > >> > >> I think the same issue with $FPATH ending/not-ending in / applies here > >> too, and for all commands in this patch. > > > > FPATH is either "" for native fat, "/" for native ext4, or <somepath>/ for > > hostfs, so this is correct. Specifically, for fat, we dont want any "/" in > > front of $FILE_foo. > > I believe FPATH can be either "", "/" or "/foo/bar" here, due to the > issue I just mentioned in the other email. FPATH is either "", "/", "/foo/bar" + "/" or "/foo/bar" + "/" + "/". The last one is not to too nice, but still correct. FPATH is *never* "/foo/bar", i.e. without trailing slash. > >>> @@ -482,6 +499,16 @@ function check_results() { > >>> > >>> + # Check directory traversal > >>> + grep -A6 "Test Case 13a " "$1" | \ > >>> + egrep -q '1048576 bytes written|update journal' > >> > >> Why is "update journal" considered successful? Surely the "n bytes > >> written" message is always printed irrespective of whether anything > >> journal-related happened? > > > > Thats a question left to the author of Test Case 11, where the fragment > > was > > copied from. > > I don't quite agree; you're adding the new test, so should make sure the > validation code makes sense. I did not claim this is correct. I just stated this problem exists already for the older test cases. > > Ext4 unfortunately is quite verbose, it inserts "File system is > > consistent" > > and "update journal finished" lines in the output. I think these lines > > where better stripped from the log prior to any further parsing. > > ext4 may print some extra output, but that doesn't mean that any of it > should be used in the validation. I think the simplest thing is to just > ignore it in the validation code. Using egrep -q '1048576 bytes written' > should do that just fine, and not get a false-positive if ext4 does say > "update journal" without saying the required "1048576 bytes written". No, this will not work for ext4 *and* fat. Every check starts with a "grep -Axx 'Test Case n'" match. If "-Axx" is extended to *always* include the "yyy bytes written", then it will work for ext4, but it will also match "Test Case n \n failed \n Test Case n+1 \n yyy bytes written". Kind Regards, Stefan
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh index cb2a765..46975ec 100755 --- a/test/fs/fs-test.sh +++ b/test/fs/fs-test.sh @@ -297,6 +297,23 @@ setenv filesize # The write should fail, but the lookup should work # Test Case 12 - Check directory traversal ${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10 + +# Read 1MB from small file +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL +# Write it via "same directory", i.e. "." dirent +# Test Case 13a - Check directory traversal +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}./${FILE_WRITE}2 \$filesize +mw.b $addr 00 100 +${PREFIX}load host${SUFFIX} $addr ${FPATH}./${FILE_WRITE}2 +# Test Case 13b - Check md5 of written to is same as the one read from +md5sum $addr \$filesize +setenv filesize +mw.b $addr 00 100 +${PREFIX}load host${SUFFIX} $addr ${FPATH}${FILE_WRITE}2 +# Test Case 13c - Check md5 of written to is same as the one read from +md5sum $addr \$filesize +setenv filesize +# reset EOF @@ -482,6 +499,16 @@ function check_results() { grep -A5 "Test Case 12 " "$1" | \ egrep -q 'Unable to write file' pass_fail "TC12: 1MB write to . - write denied" + + # Check directory traversal + grep -A6 "Test Case 13a " "$1" | \ + egrep -q '1048576 bytes written|update journal' + pass_fail "TC13: 1MB write to ./$5 - write succeeded" + check_md5 "Test Case 13b " "$1" "$2" 1 \ + "TC13: 1MB read from ./$5 - content verified" + check_md5 "Test Case 13c " "$1" "$2" 1 \ + "TC13: 1MB read from $5 - content verified" + echo "** End $1" }
<path>/<fname> and <path>/./<fname> should reference the same file. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- test/fs/fs-test.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)