[U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Stephen Warren]

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
>