* [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
@ 2014-05-19 7:26 arei.gonglei
2014-05-19 15:04 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: arei.gonglei @ 2014-05-19 7:26 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, mdroth, lcapitulino, Gonglei, Wang Rui
From: Gonglei <arei.gonglei@huawei.com>
token should be closed in all conditions.
So move CloseHandle(token) to "out" branch.
Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
qga/commands-win32.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d793dd0..e769396 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -31,7 +31,7 @@
static void acquire_privilege(const char *name, Error **errp)
{
- HANDLE token;
+ HANDLE token = NULL;
TOKEN_PRIVILEGES priv;
Error *local_err = NULL;
@@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
goto out;
}
- CloseHandle(token);
} else {
error_set(&local_err, QERR_QGA_COMMAND_FAILED,
"failed to open privilege token");
}
out:
+ if (token) {
+ CloseHandle(token);
+ }
if (local_err) {
error_propagate(errp, local_err);
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-19 7:26 [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() arei.gonglei
@ 2014-05-19 15:04 ` Eric Blake
2014-05-20 12:24 ` Yan Vugenfirer
2014-05-20 19:17 ` Luiz Capitulino
2 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2014-05-19 15:04 UTC (permalink / raw)
To: arei.gonglei, qemu-devel; +Cc: lcapitulino, mdroth, Wang Rui, armbru
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
On 05/19/2014 01:26 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> token should be closed in all conditions.
> So move CloseHandle(token) to "out" branch.
>
> Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> qga/commands-win32.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 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: 604 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-19 7:26 [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() arei.gonglei
2014-05-19 15:04 ` Eric Blake
@ 2014-05-20 12:24 ` Yan Vugenfirer
2014-05-20 19:17 ` Luiz Capitulino
2 siblings, 0 replies; 8+ messages in thread
From: Yan Vugenfirer @ 2014-05-20 12:24 UTC (permalink / raw)
To: arei.gonglei; +Cc: mdroth, lcapitulino, qemu-devel, Wang Rui, armbru
On May 19, 2014, at 10:26 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> token should be closed in all conditions.
> So move CloseHandle(token) to "out" branch.
>
> Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> qga/commands-win32.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d793dd0..e769396 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -31,7 +31,7 @@
>
> static void acquire_privilege(const char *name, Error **errp)
> {
> - HANDLE token;
> + HANDLE token = NULL;
> TOKEN_PRIVILEGES priv;
> Error *local_err = NULL;
>
> @@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
> goto out;
> }
>
> - CloseHandle(token);
> } else {
> error_set(&local_err, QERR_QGA_COMMAND_FAILED,
> "failed to open privilege token");
> }
>
> out:
> + if (token) {
> + CloseHandle(token);
> + }
> if (local_err) {
> error_propagate(errp, local_err);
> }
> --
> 1.7.12.4
>
Reviewed-by: Yan Vugenfirer <yan@daynix.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-19 7:26 [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() arei.gonglei
2014-05-19 15:04 ` Eric Blake
2014-05-20 12:24 ` Yan Vugenfirer
@ 2014-05-20 19:17 ` Luiz Capitulino
2014-05-20 19:46 ` Michael Roth
2 siblings, 1 reply; 8+ messages in thread
From: Luiz Capitulino @ 2014-05-20 19:17 UTC (permalink / raw)
To: mdroth, Wang Rui; +Cc: arei.gonglei, qemu-devel, armbru
On Mon, 19 May 2014 15:26:03 +0800
<arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> token should be closed in all conditions.
> So move CloseHandle(token) to "out" branch.
Looks good to me. Michael, are you going to pick this one?
>
> Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> qga/commands-win32.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d793dd0..e769396 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -31,7 +31,7 @@
>
> static void acquire_privilege(const char *name, Error **errp)
> {
> - HANDLE token;
> + HANDLE token = NULL;
> TOKEN_PRIVILEGES priv;
> Error *local_err = NULL;
>
> @@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
> goto out;
> }
>
> - CloseHandle(token);
> } else {
> error_set(&local_err, QERR_QGA_COMMAND_FAILED,
> "failed to open privilege token");
> }
>
> out:
> + if (token) {
> + CloseHandle(token);
> + }
> if (local_err) {
> error_propagate(errp, local_err);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-20 19:17 ` Luiz Capitulino
@ 2014-05-20 19:46 ` Michael Roth
2014-05-21 9:00 ` Yan Vugenfirer
0 siblings, 1 reply; 8+ messages in thread
From: Michael Roth @ 2014-05-20 19:46 UTC (permalink / raw)
To: Luiz Capitulino, Wang Rui; +Cc: arei.gonglei, qemu-devel, armbru
Quoting Luiz Capitulino (2014-05-20 14:17:42)
> On Mon, 19 May 2014 15:26:03 +0800
> <arei.gonglei@huawei.com> wrote:
>
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > token should be closed in all conditions.
> > So move CloseHandle(token) to "out" branch.
>
> Looks good to me. Michael, are you going to pick this one?
Sure I'll queue it. Though i'm a bit surprised OpenProcessToken() will still
return an open handle on failure. Is there any confirmation a handle is
actually getting leaked here, as opposed to just returning a handle that's
already been closed?
If it's the latter case we might end up closing it twice after the change,
so just want to confirm.
>
> >
> > Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> > qga/commands-win32.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index d793dd0..e769396 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -31,7 +31,7 @@
> >
> > static void acquire_privilege(const char *name, Error **errp)
> > {
> > - HANDLE token;
> > + HANDLE token = NULL;
> > TOKEN_PRIVILEGES priv;
> > Error *local_err = NULL;
> >
> > @@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
> > goto out;
> > }
> >
> > - CloseHandle(token);
> > } else {
> > error_set(&local_err, QERR_QGA_COMMAND_FAILED,
> > "failed to open privilege token");
> > }
> >
> > out:
> > + if (token) {
> > + CloseHandle(token);
> > + }
> > if (local_err) {
> > error_propagate(errp, local_err);
> > }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-20 19:46 ` Michael Roth
@ 2014-05-21 9:00 ` Yan Vugenfirer
2014-05-21 10:11 ` Wangrui (K)
0 siblings, 1 reply; 8+ messages in thread
From: Yan Vugenfirer @ 2014-05-21 9:00 UTC (permalink / raw)
To: Michael Roth; +Cc: qemu-devel, armbru, Wang Rui, arei.gonglei, Luiz Capitulino
On May 20, 2014, at 10:46 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Quoting Luiz Capitulino (2014-05-20 14:17:42)
>> On Mon, 19 May 2014 15:26:03 +0800
>> <arei.gonglei@huawei.com> wrote:
>>
>>> From: Gonglei <arei.gonglei@huawei.com>
>>>
>>> token should be closed in all conditions.
>>> So move CloseHandle(token) to "out" branch.
>>
>> Looks good to me. Michael, are you going to pick this one?
>
> Sure I'll queue it. Though i'm a bit surprised OpenProcessToken() will still
> return an open handle on failure. Is there any confirmation a handle is
> actually getting leaked here, as opposed to just returning a handle that's
> already been closed?
It won’t return a handle if it failed (I actually was going to reject the patch because of it) - but if you look closely at the code you will see error cases after OpenProcessToken was successful where we jump out of the if scope and then CloseHandle won’t be called.
Best regards,
Yan.
>
> If it's the latter case we might end up closing it twice after the change,
> so just want to confirm.
>
>>
>>>
>>> Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>> qga/commands-win32.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>>> index d793dd0..e769396 100644
>>> --- a/qga/commands-win32.c
>>> +++ b/qga/commands-win32.c
>>> @@ -31,7 +31,7 @@
>>>
>>> static void acquire_privilege(const char *name, Error **errp)
>>> {
>>> - HANDLE token;
>>> + HANDLE token = NULL;
>>> TOKEN_PRIVILEGES priv;
>>> Error *local_err = NULL;
>>>
>>> @@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
>>> goto out;
>>> }
>>>
>>> - CloseHandle(token);
>>> } else {
>>> error_set(&local_err, QERR_QGA_COMMAND_FAILED,
>>> "failed to open privilege token");
>>> }
>>>
>>> out:
>>> + if (token) {
>>> + CloseHandle(token);
>>> + }
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> }
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-05-21 9:00 ` Yan Vugenfirer
@ 2014-05-21 10:11 ` Wangrui (K)
0 siblings, 0 replies; 8+ messages in thread
From: Wangrui (K) @ 2014-05-21 10:11 UTC (permalink / raw)
To: Yan Vugenfirer, Michael Roth
Cc: armbru@redhat.com, Gonglei (Arei), qemu-devel@nongnu.org,
Luiz Capitulino
> -----Original Message-----
> From: Yan Vugenfirer [mailto:yvugenfi@redhat.com]
> Sent: Wednesday, May 21, 2014 5:01 PM
> To: Michael Roth
> Cc: Luiz Capitulino; Wangrui (K); Gonglei (Arei); qemu-devel@nongnu.org;
> armbru@redhat.com
> Subject: Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
>
>
> On May 20, 2014, at 10:46 PM, Michael Roth <mdroth@linux.vnet.ibm.com>
> wrote:
>
> > Quoting Luiz Capitulino (2014-05-20 14:17:42)
> >> On Mon, 19 May 2014 15:26:03 +0800
> >> <arei.gonglei@huawei.com> wrote:
> >>
> >>> From: Gonglei <arei.gonglei@huawei.com>
> >>>
> >>> token should be closed in all conditions.
> >>> So move CloseHandle(token) to "out" branch.
> >>
> >> Looks good to me. Michael, are you going to pick this one?
> >
> > Sure I'll queue it. Though i'm a bit surprised OpenProcessToken() will still
> > return an open handle on failure. Is there any confirmation a handle is
> > actually getting leaked here, as opposed to just returning a handle that's
> > already been closed?
>
> It won't return a handle if it failed (I actually was going to reject the patch
> because of it) - but if you look closely at the code you will see error cases after
> OpenProcessToken was successful where we jump out of the if scope and then
> CloseHandle won't be called.
>
Yep.
The two "goto out"s are in the condition that OpenProcessToken returns successfully.
> Best regards,
> Yan.
>
> >
> > If it's the latter case we might end up closing it twice after the change,
> > so just want to confirm.
> >
> >>
> >>>
> >>> Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >>> ---
> >>> qga/commands-win32.c | 6 ++++--
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> >>> index d793dd0..e769396 100644
> >>> --- a/qga/commands-win32.c
> >>> +++ b/qga/commands-win32.c
> >>> @@ -31,7 +31,7 @@
> >>>
> >>> static void acquire_privilege(const char *name, Error **errp)
> >>> {
> >>> - HANDLE token;
> >>> + HANDLE token = NULL;
> >>> TOKEN_PRIVILEGES priv;
> >>> Error *local_err = NULL;
> >>>
> >>> @@ -53,13 +53,15 @@ static void acquire_privilege(const char *name,
> Error **errp)
> >>> goto out;
> >>> }
> >>>
> >>> - CloseHandle(token);
> >>> } else {
> >>> error_set(&local_err, QERR_QGA_COMMAND_FAILED,
> >>> "failed to open privilege token");
> >>> }
> >>>
> >>> out:
> >>> + if (token) {
> >>> + CloseHandle(token);
> >>> + }
> >>> if (local_err) {
> >>> error_propagate(errp, local_err);
> >>> }
> >
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PULL 0/1] qemu-ga fixes for win32
@ 2014-06-05 19:41 Michael Roth
2014-06-05 19:41 ` [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() Michael Roth
0 siblings, 1 reply; 8+ messages in thread
From: Michael Roth @ 2014-06-05 19:41 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, arei.gonglei, qemu-stable
The following changes since commit e00fcfeab3d452cba3d0a08991a39ab15df66424:
Merge remote-tracking branch 'remotes/awilliam/tags/vfio-pci-for-qemu-20140602.0' into staging (2014-06-03 14:37:43 +0100)
are available in the git repository at:
git://github.com/mdroth/qemu.git qga-pull-2014-06-05
for you to fetch changes up to 374044f08fe18a18469b981812cd8695f5b3569c:
qga: Fix handle fd leak in acquire_privilege() (2014-06-03 15:07:59 -0500)
----------------------------------------------------------------
Gonglei (1):
qga: Fix handle fd leak in acquire_privilege()
qga/commands-win32.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
2014-06-05 19:41 [Qemu-devel] [PULL 0/1] qemu-ga fixes for win32 Michael Roth
@ 2014-06-05 19:41 ` Michael Roth
0 siblings, 0 replies; 8+ messages in thread
From: Michael Roth @ 2014-06-05 19:41 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, arei.gonglei, qemu-stable
From: Gonglei <arei.gonglei@huawei.com>
token should be closed in all conditions.
So move CloseHandle(token) to "out" branch.
Signed-off-by: Wang Rui <moon.wangrui@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qga/commands-win32.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d793dd0..e769396 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -31,7 +31,7 @@
static void acquire_privilege(const char *name, Error **errp)
{
- HANDLE token;
+ HANDLE token = NULL;
TOKEN_PRIVILEGES priv;
Error *local_err = NULL;
@@ -53,13 +53,15 @@ static void acquire_privilege(const char *name, Error **errp)
goto out;
}
- CloseHandle(token);
} else {
error_set(&local_err, QERR_QGA_COMMAND_FAILED,
"failed to open privilege token");
}
out:
+ if (token) {
+ CloseHandle(token);
+ }
if (local_err) {
error_propagate(errp, local_err);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-05 19:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-19 7:26 [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() arei.gonglei
2014-05-19 15:04 ` Eric Blake
2014-05-20 12:24 ` Yan Vugenfirer
2014-05-20 19:17 ` Luiz Capitulino
2014-05-20 19:46 ` Michael Roth
2014-05-21 9:00 ` Yan Vugenfirer
2014-05-21 10:11 ` Wangrui (K)
-- strict thread matches above, loose matches on Subject: below --
2014-06-05 19:41 [Qemu-devel] [PULL 0/1] qemu-ga fixes for win32 Michael Roth
2014-06-05 19:41 ` [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege() Michael Roth
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).