[Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Michael Roth]

From: Gonglei 

token should be closed in all conditions.
So move CloseHandle(token) to "out" branch.

Signed-off-by: Wang Rui 
Signed-off-by: Gonglei 
Signed-off-by: Michael Roth 
---
 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

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Wangrui (K)]

> -Original Message-
> From: Yan Vugenfirer [mailto:yvuge...@redhat.com]
> Sent: Wednesday, May 21, 2014 5:01 PM
> To: Michael Roth
> Cc: Luiz Capitulino; Wangrui (K); Gonglei (Arei); qemu-devel@nongnu.org;
> arm...@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 
> wrote:
> 
> > Quoting Luiz Capitulino (2014-05-20 14:17:42)
> >> On Mon, 19 May 2014 15:26:03 +0800
> >>  wrote:
> >>
> >>> From: Gonglei 
> >>>
> >>> 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 
> >>> Signed-off-by: Gonglei 
> >>> ---
> >>> 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);
> >>> }
> >
> >

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Yan Vugenfirer]

On May 20, 2014, at 10:46 PM, Michael Roth  wrote:

> Quoting Luiz Capitulino (2014-05-20 14:17:42)
>> On Mon, 19 May 2014 15:26:03 +0800
>>  wrote:
>> 
>>> From: Gonglei 
>>> 
>>> 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 
>>> Signed-off-by: Gonglei 
>>> ---
>>> 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);
>>> }
> 
> 

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Michael Roth]

Quoting Luiz Capitulino (2014-05-20 14:17:42)
> On Mon, 19 May 2014 15:26:03 +0800
>  wrote:
> 
> > From: Gonglei 
> > 
> > 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 
> > Signed-off-by: Gonglei 
> > ---
> >  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);
> >  }

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Luiz Capitulino]

On Mon, 19 May 2014 15:26:03 +0800
 wrote:

> From: Gonglei 
> 
> 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 
> Signed-off-by: Gonglei 
> ---
>  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);
>  }

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Yan Vugenfirer]

On May 19, 2014, at 10:26 AM, arei.gong...@huawei.com wrote:

> From: Gonglei 
> 
> token should be closed in all conditions.
> So move CloseHandle(token) to "out" branch.
> 
> Signed-off-by: Wang Rui 
> Signed-off-by: Gonglei 
> ---
> 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 

Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[Eric Blake]

On 05/19/2014 01:26 AM, arei.gong...@huawei.com wrote:
> From: Gonglei 
> 
> token should be closed in all conditions.
> So move CloseHandle(token) to "out" branch.
> 
> Signed-off-by: Wang Rui 
> Signed-off-by: Gonglei 
> ---
>  qga/commands-win32.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()

[arei.gonglei]

From: Gonglei 

token should be closed in all conditions.
So move CloseHandle(token) to "out" branch.

Signed-off-by: Wang Rui 
Signed-off-by: Gonglei 
---
 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