Re: [U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block
On Dienstag, 13. September 2016 12:33:15 CEST Stephen Warren wrote:
> On 09/12/2016 03:48 PM, Stefan Bruens wrote:
> > On Montag, 12. September 2016 12:39:35 CEST you wrote:
> >> On 09/11/2016 02:46 PM, Stefan Brüns wrote:
> >>> This is a regression test for a crash happening if the first dirent
> >>> in the block matches. Code tried to access a predecessor entry which
> >>> does not exist.
> >>> The crash happened for any block, but "." is always the first entry in
> >>> the first directory block and thus easy to check for.
> >>>
> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >>>
> >>> +# Next test case checks writing a file whose dirent
> >>> +# is the first in the block, which is always true for "."
> >>> +# The write should fail, but the lookup should work
> >>> +# Test Case 12 - Check directory traversal
> >>> +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
> >>
> >> Doesn't that attempt to write a file named ".", which would end up
> >> over-writing the directory content? Unless I'm misunderstanding, that
> >> doesn't seem like a good idea.
> >>
> >> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
> >> "." to some of those won't work the same way as some others.
> >
> > "something" can not happen.
>
> I don't see any code that prevents that. I think it's fairer to say that
> nothing currently happens to call the function with FPATH with a
> trailing /. Someone could easily edit the script later and add such a
> call without knowing about the undocumented restriction.
>
> I note that in patch 1, the following logic:
>
> + if [ ! -z "$6" ]; then
> + FPATH=${6}/${FPATH}
> fi
>
> ... ends up with FPATH having a leading / for the second invocation of
> test_image by the main script body, since ${6} has the value
> `pwd`/$MOUNT_DIR. That seems to violate FAT's requirement for pathnames
> not to have a leading /.
There are 6 different test runs: { sb, fs, nonfs } x { fat, ext4 }
fs and nonfs can be treated the same way, its just e.g. "ext4ls", "ls" or
"fatls", "ls" resp. Thats what I called "native" in the other mail.
Now in the "sb" cases, it does not matter if the mounted image is fat or ext4
- it always uses the full absolute path to the mountpoint.
The specific requirements of ext4 (absolute path) and fat (relative path) only
apply to the nonfs and fs test runs.
Kind regards,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block
On 09/12/2016 03:48 PM, Stefan Bruens wrote:
On Montag, 12. September 2016 12:39:35 CEST you wrote:
On 09/11/2016 02:46 PM, Stefan Brüns wrote:
This is a regression test for a crash happening if the first dirent
in the block matches. Code tried to access a predecessor entry which
does not exist.
The crash happened for any block, but "." is always the first entry in
the first directory block and thus easy to check for.
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
+# Next test case checks writing a file whose dirent
+# is the first in the block, which is always true for "."
+# The write should fail, but the lookup should work
+# Test Case 12 - Check directory traversal
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
Doesn't that attempt to write a file named ".", which would end up
over-writing the directory content? Unless I'm misunderstanding, that
doesn't seem like a good idea.
Also, ${FPATH} might be "", "/", "something", or "something/". Appending
"." to some of those won't work the same way as some others.
"something" can not happen.
I don't see any code that prevents that. I think it's fairer to say that
nothing currently happens to call the function with FPATH with a
trailing /. Someone could easily edit the script later and add such a
call without knowing about the undocumented restriction.
I note that in patch 1, the following logic:
+ if [ ! -z "$6" ]; then
+ FPATH=${6}/${FPATH}
fi
... ends up with FPATH having a leading / for the second invocation of
test_image by the main script body, since ${6} has the value
`pwd`/$MOUNT_DIR. That seems to violate FAT's requirement for pathnames
not to have a leading /.
> The other three cases are exactly what is wanted
here, as fat currently needs a relative path, with CWD being "/", and ext
explicitly wants an absolute path.
@@ -471,6 +477,11 @@ function check_results() {
+ # Check lookup of 'dot' directory
+ grep -A5 "Test Case 12 " "$1" | \
+ egrep -q 'Unable to write file'
+ pass_fail "TC12: 1MB write to . - write denied"
Oh, I see; this expects the write to be denied since the destination is
a directory. Perhaps that's OK. I'd rather see a read attempt though, to
guarantee that even if the access does end up being allowed, the FS
isn't trashed for any future tests that may be added afterwards.
U-Boot master/2016-09 crashes here without the pending ext4 fixes.
> IMHO after
a failed test case, everything afterwards is invalid anyway, if the same fs
image is used.
That's fair.
As soon as the image is modified, tests are no longer idempotent. Block
allocation order may change, anything that allocates any resource changes the
image.
Atempting a write here is done as it actually exercises a different code path.
"ext4load host 0:0 0 /." reports "File not found", "ext4write host 0:0 0 /."
crashes.
Kind regards,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block
On Montag, 12. September 2016 12:39:35 CEST you wrote:
> On 09/11/2016 02:46 PM, Stefan Brüns wrote:
> > This is a regression test for a crash happening if the first dirent
> > in the block matches. Code tried to access a predecessor entry which
> > does not exist.
> > The crash happened for any block, but "." is always the first entry in
> > the first directory block and thus easy to check for.
> >
> > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >
> > +# Next test case checks writing a file whose dirent
> > +# is the first in the block, which is always true for "."
> > +# The write should fail, but the lookup should work
> > +# Test Case 12 - Check directory traversal
> > +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
>
> Doesn't that attempt to write a file named ".", which would end up
> over-writing the directory content? Unless I'm misunderstanding, that
> doesn't seem like a good idea.
>
> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
> "." to some of those won't work the same way as some others.
"something" can not happen. The other three cases are exactly what is wanted
here, as fat currently needs a relative path, with CWD being "/", and ext
explicitly wants an absolute path.
> > @@ -471,6 +477,11 @@ function check_results() {
> >
> > + # Check lookup of 'dot' directory
> > + grep -A5 "Test Case 12 " "$1" | \
> > + egrep -q 'Unable to write file'
> > + pass_fail "TC12: 1MB write to . - write denied"
>
> Oh, I see; this expects the write to be denied since the destination is
> a directory. Perhaps that's OK. I'd rather see a read attempt though, to
> guarantee that even if the access does end up being allowed, the FS
> isn't trashed for any future tests that may be added afterwards.
U-Boot master/2016-09 crashes here without the pending ext4 fixes. IMHO after
a failed test case, everything afterwards is invalid anyway, if the same fs
image is used.
As soon as the image is modified, tests are no longer idempotent. Block
allocation order may change, anything that allocates any resource changes the
image.
Atempting a write here is done as it actually exercises a different code path.
"ext4load host 0:0 0 /." reports "File not found", "ext4write host 0:0 0 /."
crashes.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block
On 09/11/2016 02:46 PM, Stefan Brüns wrote:
This is a regression test for a crash happening if the first dirent
in the block matches. Code tried to access a predecessor entry which
does not exist.
The crash happened for any block, but "." is always the first entry in
the first directory block and thus easy to check for.
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
+# Next test case checks writing a file whose dirent
+# is the first in the block, which is always true for "."
+# The write should fail, but the lookup should work
+# Test Case 12 - Check directory traversal
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
Doesn't that attempt to write a file named ".", which would end up
over-writing the directory content? Unless I'm misunderstanding, that
doesn't seem like a good idea.
Also, ${FPATH} might be "", "/", "something", or "something/". Appending
"." to some of those won't work the same way as some others.
@@ -471,6 +477,11 @@ function check_results() {
+ # Check lookup of 'dot' directory
+ grep -A5 "Test Case 12 " "$1" | \
+ egrep -q 'Unable to write file'
+ pass_fail "TC12: 1MB write to . - write denied"
Oh, I see; this expects the write to be denied since the destination is
a directory. Perhaps that's OK. I'd rather see a read attempt though, to
guarantee that even if the access does end up being allowed, the FS
isn't trashed for any future tests that may be added afterwards.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block
This is a regression test for a crash happening if the first dirent
in the block matches. Code tried to access a predecessor entry which
does not exist.
The crash happened for any block, but "." is always the first entry in
the first directory block and thus easy to check for.
Signed-off-by: Stefan Brüns
---
test/fs/fs-test.sh | 11 +++
1 file changed, 11 insertions(+)
diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index 12450ed..cb2a765 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -291,6 +291,12 @@ ${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_WRITE
md5sum $addr \$filesize
setenv filesize
#
+
+# Next test case checks writing a file whose dirent
+# is the first in the block, which is always true for "."
+# The write should fail, but the lookup should work
+# Test Case 12 - Check directory traversal
+${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
reset
EOF
@@ -471,6 +477,11 @@ function check_results() {
pass_fail "TC11: 1MB write to $5 - write succeeded"
check_md5 "Test Case 11b " "$1" "$2" 1 \
"TC11: 1MB write to $5 - content verified"
+
+ # Check lookup of 'dot' directory
+ grep -A5 "Test Case 12 " "$1" | \
+ egrep -q 'Unable to write file'
+ pass_fail "TC12: 1MB write to . - write denied"
echo "** End $1"
}
--
2.10.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
