* [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory @ 2018-07-18 15:28 Thomas Huth 2018-07-18 22:58 ` Philippe Mathieu-Daudé 2018-07-23 14:33 ` Kevin Wolf 0 siblings, 2 replies; 6+ messages in thread From: Thomas Huth @ 2018-07-18 15:28 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: Max Reitz, qemu-devel 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 <thuth@redhat.com> --- 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); return -1; } mapping = array_get(&(s->mapping), i); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory 2018-07-18 15:28 [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory Thomas Huth @ 2018-07-18 22:58 ` Philippe Mathieu-Daudé 2018-07-23 14:33 ` Kevin Wolf 1 sibling, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2018-07-18 22:58 UTC (permalink / raw) To: Thomas Huth, Kevin Wolf, qemu-block; +Cc: qemu-devel, Max Reitz On 07/18/2018 12:28 PM, Thomas Huth wrote: > 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 <thuth@redhat.com> > --- > 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); > return -1; > } > mapping = array_get(&(s->mapping), i); > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory 2018-07-18 15:28 [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory Thomas Huth 2018-07-18 22:58 ` Philippe Mathieu-Daudé @ 2018-07-23 14:33 ` Kevin Wolf 2018-07-24 9:56 ` Thomas Huth 1 sibling, 1 reply; 6+ messages in thread From: Kevin Wolf @ 2018-07-23 14:33 UTC (permalink / raw) To: Thomas Huth; +Cc: qemu-block, Max Reitz, qemu-devel 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 <thuth@redhat.com> > --- > 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. > 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? Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory 2018-07-23 14:33 ` Kevin Wolf @ 2018-07-24 9:56 ` Thomas Huth 2018-07-24 10:49 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Thomas Huth @ 2018-07-24 9:56 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, Max Reitz, qemu-devel 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 <thuth@redhat.com> >> --- >> 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? Thomas ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory 2018-07-24 9:56 ` Thomas Huth @ 2018-07-24 10:49 ` Kevin Wolf 2018-07-24 11:38 ` Thomas Huth 0 siblings, 1 reply; 6+ messages in thread From: Kevin Wolf @ 2018-07-24 10:49 UTC (permalink / raw) To: Thomas Huth; +Cc: qemu-block, Max Reitz, qemu-devel 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 <thuth@redhat.com> > >> --- > >> 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. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory 2018-07-24 10:49 ` Kevin Wolf @ 2018-07-24 11:38 ` Thomas Huth 0 siblings, 0 replies; 6+ messages in thread From: Thomas Huth @ 2018-07-24 11:38 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, Max Reitz, qemu-devel 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 <thuth@redhat.com> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-24 11:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-18 15:28 [Qemu-devel] [PATCH] block/vvfat: Fix crash when reporting error about too many files in directory Thomas Huth 2018-07-18 22:58 ` Philippe Mathieu-Daudé 2018-07-23 14:33 ` Kevin Wolf 2018-07-24 9:56 ` Thomas Huth 2018-07-24 10:49 ` Kevin Wolf 2018-07-24 11:38 ` Thomas Huth
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).