| Message ID | 1411011222-5116-1-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
zhanghailiang <zhang.zhanghailiang@huawei.com> writes: > If readdir_r fails, error_setg_errno will reference the freed > pointer *dirpath*. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 09/17/2014 09:33 PM, zhanghailiang wrote: > If readdir_r fails, error_setg_errno will reference the freed > pointer *dirpath*. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > qga/commands-posix.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > for (;;) { > if (readdir_r(dir, &entry, &result) != 0) { Eww. We're using readdir_r? That's an inherently broken interface, which can risk buffer overflow. readdir should be preferred. http://austingroupbugs.net/view.php?id=696
On 2014/9/18 20:17, Eric Blake wrote: > On 09/17/2014 09:33 PM, zhanghailiang wrote: >> If readdir_r fails, error_setg_errno will reference the freed >> pointer *dirpath*. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> --- >> qga/commands-posix.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > >> for (;;) { >> if (readdir_r(dir, &entry, &result) != 0) { > > Eww. We're using readdir_r? That's an inherently broken interface, > which can risk buffer overflow. readdir should be preferred. > > http://austingroupbugs.net/view.php?id=696 > Yes, it is! Should i fix it in this patch together?;) Thanks, zhanghailiang
On 09/18/2014 06:42 AM, zhanghailiang wrote: > On 2014/9/18 20:17, Eric Blake wrote: >> On 09/17/2014 09:33 PM, zhanghailiang wrote: >>> If readdir_r fails, error_setg_errno will reference the freed >>> pointer *dirpath*. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> --- >>> qga/commands-posix.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >> >>> for (;;) { >>> if (readdir_r(dir, &entry, &result) != 0) { >> >> Eww. We're using readdir_r? That's an inherently broken interface, >> which can risk buffer overflow. readdir should be preferred. >> >> http://austingroupbugs.net/view.php?id=696 >> > > Yes, it is! Should i fix it in this patch together?;) Switching to readdir would be welcome, and would probably be enough of a rewrite that it would also fix the use-after-free without trying to break it into two patches. You're welcome to try that as a v2.
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 7eed7f4..3082eae 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -965,7 +965,6 @@ 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) { @@ -977,10 +976,12 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath, } if (entry.d_type == DT_LNK) { + char *path; + 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); + 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 +989,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*. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- qga/commands-posix.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)