* Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r
2014-09-19 3:09 [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r zhanghailiang
@ 2014-09-26 8:32 ` zhanghailiang
2014-09-26 15:40 ` Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: zhanghailiang @ 2014-09-26 8:32 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, armbru, luonengjun, peter.huangpeng, qemu-stable,
lcapitulino
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);
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r
2014-09-19 3:09 [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r zhanghailiang
2014-09-26 8:32 ` zhanghailiang
@ 2014-09-26 15:40 ` Paolo Bonzini
2014-09-28 6:06 ` zhanghailiang
2014-09-26 15:45 ` Eric Blake
2014-10-01 17:47 ` Michael Roth
3 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2014-09-26 15:40 UTC (permalink / raw)
To: zhanghailiang, qemu-devel
Cc: mdroth, armbru, qemu-stable, luonengjun, peter.huangpeng,
lcapitulino
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r
2014-09-26 15:40 ` Paolo Bonzini
@ 2014-09-28 6:06 ` zhanghailiang
0 siblings, 0 replies; 6+ messages in thread
From: zhanghailiang @ 2014-09-28 6:06 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: mdroth, armbru, qemu-stable, luonengjun, peter.huangpeng,
lcapitulino
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
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r
2014-09-19 3:09 [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r zhanghailiang
2014-09-26 8:32 ` zhanghailiang
2014-09-26 15:40 ` Paolo Bonzini
@ 2014-09-26 15:45 ` Eric Blake
2014-10-01 17:47 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2014-09-26 15:45 UTC (permalink / raw)
To: zhanghailiang, qemu-devel
Cc: mdroth, armbru, luonengjun, peter.huangpeng, qemu-stable,
lcapitulino
[-- Attachment #1: Type: text/plain, Size: 634 bytes --]
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>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r
2014-09-19 3:09 [Qemu-devel] [PATCH v2] qga: Rewrite code where using readdir_r zhanghailiang
` (2 preceding siblings ...)
2014-09-26 15:45 ` Eric Blake
@ 2014-10-01 17:47 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Michael Roth @ 2014-10-01 17:47 UTC (permalink / raw)
To: zhanghailiang, qemu-devel
Cc: armbru, luonengjun, peter.huangpeng, qemu-stable, lcapitulino
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
^ permalink raw reply [flat|nested] 6+ messages in thread