Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r

[zhanghailiang]

On 2014/9/26 23:40, Paolo Bonzini wrote:
> Il 19/09/2014 05:09, zhanghailiang ha scritto:
> > If readdir_r fails, error_setg_errno will reference the freed
> > pointer *dirpath*.
> >
> > Moreover, readdir_r may cause a buffer overflow, using readdir instead.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > ---
> >   v2:
> > - Switch readdir_r to readdir (Comment of Eric Blake)
> > ---
> >   qga/commands-posix.c | 27 +++++++++++++++------------
> >   1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 7eed7f4..f6f3e3c 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -956,7 +956,7 @@ static void build_guest_fsinfo_for_virtual_device(char 
> > const *syspath,
> >   {
> >       DIR *dir;
> >       char *dirpath;
> > -    struct dirent entry, *result;
> > +    struct dirent *entry;
> >
> >       dirpath = g_strdup_printf("%s/slaves", syspath);
> >       dir = opendir(dirpath);
> > @@ -965,22 +965,24 @@ static void build_guest_fsinfo_for_virtual_device(char 
> > const *syspath,
> >           g_free(dirpath);
> >           return;
> >       }
> > -    g_free(dirpath);
> >
> >       for (;;) {
> > -        if (readdir_r(dir, &entry, &result) != 0) {
> > -            error_setg_errno(errp, errno, "readdir_r(\"%s\")", dirpath);
> > -            break;
> > -        }
> > -        if (!result) {
> > +        errno = 0;
> > +        entry = readdir(dir);
> > +        if (entry == NULL) {
> > +            if (errno) {
> > +                error_setg_errno(errp, errno, "readdir(\"%s\")", dirpath);
> > +            }
> >               break;
> >           }
> >
> > -        if (entry.d_type == DT_LNK) {
> > -            g_debug(" slave device '%s'", entry.d_name);
> > -            dirpath = g_strdup_printf("%s/slaves/%s", syspath, entry.d_name);
> > -            build_guest_fsinfo_for_device(dirpath, fs, errp);
> > -            g_free(dirpath);
> > +        if (entry->d_type == DT_LNK) {
> > +            char *path;
> > +
> > +            g_debug(" slave device '%s'", entry->d_name);
> > +            path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
> > +            build_guest_fsinfo_for_device(path, fs, errp);
> > +            g_free(path);
> >
> >               if (*errp) {
> >                   break;
> > @@ -988,6 +990,7 @@ static void build_guest_fsinfo_for_virtual_device(char 
> > const *syspath,
> >           }
> >       }
> >
> > +    g_free(dirpath);
> >       closedir(dir);
> >   }
>
> Thanks,
>
> Reviewed-by: Paolo Bonzini <address@hidden>
>
> Michael Roth will pick this up.

OK, Thanks!

> Paolo
>
> .