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

[Wangrui (K)]

> -----Original Message-----
> From: Yan Vugenfirer [mailto:address@hidden
> Sent: Wednesday, May 21, 2014 5:01 PM
> To: Michael Roth
> Cc: Luiz Capitulino; Wangrui (K); Gonglei (Arei); address@hidden;
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in 
> acquire_privilege()
> 
> 
> On May 20, 2014, at 10:46 PM, Michael Roth <address@hidden>
> wrote:
> 
> > Quoting Luiz Capitulino (2014-05-20 14:17:42)
> >> On Mon, 19 May 2014 15:26:03 +0800
> >> <address@hidden> wrote:
> >>
> >>> From: Gonglei <address@hidden>
> >>>
> >>> 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 <address@hidden>
> >>> Signed-off-by: Gonglei <address@hidden>
> >>> ---
> >>> 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);
> >>>     }
> >
> >