Message ID | 1411096150-3044-1-git-send-email-zhang.zhanghailiang@huawei.com |
---|---|
State | New |
Headers | show |
Hi, Ping...,plus;) This is a bug fix. Thanks, zhanghailiang On 2014/9/19 11:09, zhanghailiang wrote: > 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 <zhang.zhanghailiang@huawei.com> > --- > 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); > } > >
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 <zhang.zhanghailiang@huawei.com> > --- > 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 <pbonzini@redhat.com> Michael Roth will pick this up. Paolo
On 09/18/2014 09:09 PM, zhanghailiang wrote: > 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 <zhang.zhanghailiang@huawei.com> > --- > v2: > - Switch readdir_r to readdir (Comment of Eric Blake) > --- > qga/commands-posix.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
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 <zhang.zhanghailiang@huawei.com> >> --- >> 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 <pbonzini@redhat.com> > > Michael Roth will pick this up. > OK, Thanks! > Paolo > > . >
Quoting zhanghailiang (2014-09-18 22:09:10) > 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 <zhang.zhanghailiang@huawei.com> Thanks, applied to qga tree: https://github.com/mdroth/qemu/commits/qga > --- > 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); > } > > -- > 1.7.12.4
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); }
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 <zhang.zhanghailiang@huawei.com> --- v2: - Switch readdir_r to readdir (Comment of Eric Blake) --- qga/commands-posix.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)