Re: [Qemu-block] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory

[Thomas Huth]

On 24.07.2018 12:49, Kevin Wolf wrote:
> Am 24.07.2018 um 11:56 hat Thomas Huth geschrieben:
>> On 23.07.2018 16:33, Kevin Wolf wrote:
>>> Am 18.07.2018 um 17:28 hat Thomas Huth geschrieben:
>>>> When using the vvfat driver with a directory that contains too many files,
>>>> QEMU currently crashes. We are trying to print the wrong path variable 
>>>> here.
>>>>
>>>> Signed-off-by: Thomas Huth <address@hidden>
>>>> ---
>>>>  block/vvfat.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>>> index fc41841..6ae7458 100644
>>>> --- a/block/vvfat.c
>>>> +++ b/block/vvfat.c
>>>> @@ -975,8 +975,7 @@ static int init_directories(BDRVVVFATState* s,
>>>>          if (mapping->mode & MODE_DIRECTORY) {
>>>>              mapping->begin = cluster;
>>>>              if(read_directory(s, i)) {
>>>> -                error_setg(errp, "Could not read directory %s",
>>>> -                           mapping->path);
>>>> +                error_setg(errp, "Could not read directory \"%s\"", 
>>>> s->path);
>>>
>>> Hm, I'm not sure if that's right. Before this patch we were printing
>>> the name of the subdirectory that couldn't be loaded, now it's the
>>> parent directory.
>>>
>>> My test case where this difference is visible is a subdirectory with
>>> chmod 000.
>>
>> Right.
>>
>>>>                  return -1;
>>>>              }
>>>>              mapping = array_get(&(s->mapping), i);
>>>
>>> Maybe the right solution would be moving the reloading of mapping to
>>> between the read_directory() call and the error path?
>>
>> No, that does not work either. The problem seems to be that
>> read_directory() is changing the mapping->path pointer to something
>> invalid in between, but I've been unable to track it down where it
>> happens. This patch here seems to work for me, though:
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index fc41841..f2e7d50 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -973,10 +973,10 @@ static int init_directories(BDRVVVFATState* s,
>>          mapping = array_get(&(s->mapping), i);
>>
>>          if (mapping->mode & MODE_DIRECTORY) {
>> +            char *path = mapping->path;
>>              mapping->begin = cluster;
>>              if(read_directory(s, i)) {
>> -                error_setg(errp, "Could not read directory %s",
>> -                           mapping->path);
>> +                error_setg(errp, "Could not read directory %s", path);
>>                  return -1;
>>              }
>>              mapping = array_get(&(s->mapping), i);
>>
>> Does this look reasonable for you, too?
> 
> I can't say I understand what's going on (the change I suggested did
> work for my test case, without valgrind errors), but the above patch
> doesn't look wrong to me at least.

FWIW, my test:

mkdir /tmp/vvfattest
cd /tmp/vvfattest
for ((x=0;x<=513;x++)); do mkdir $x; done
qemu-system-x86_64 -drive \
 file.driver=vvfat,file.dir=.,read-only=on,media=cdrom

I'll send a proper patch with above suggestion.

 Thomas