Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
On May 20, 2014, at 10:46 PM, Michael Roth mdr...@linux.vnet.ibm.com wrote:
Quoting Luiz Capitulino (2014-05-20 14:17:42)
On Mon, 19 May 2014 15:26:03 +0800
arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@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.wang...@huawei.com
Signed-off-by: Gonglei arei.gong...@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);
}
Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
-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 mdr...@linux.vnet.ibm.com
wrote:
Quoting Luiz Capitulino (2014-05-20 14:17:42)
On Mon, 19 May 2014 15:26:03 +0800
arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@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 outs 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.wang...@huawei.com
Signed-off-by: Gonglei arei.gong...@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);
}
Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
On May 19, 2014, at 10:26 AM, arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@huawei.com
token should be closed in all conditions.
So move CloseHandle(token) to out branch.
Signed-off-by: Wang Rui moon.wang...@huawei.com
Signed-off-by: Gonglei arei.gong...@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 y...@daynix.com
Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
On Mon, 19 May 2014 15:26:03 +0800
arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@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.wang...@huawei.com
Signed-off-by: Gonglei arei.gong...@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);
}
Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
Quoting Luiz Capitulino (2014-05-20 14:17:42)
On Mon, 19 May 2014 15:26:03 +0800
arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@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.wang...@huawei.com
Signed-off-by: Gonglei arei.gong...@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);
}
Re: [Qemu-devel] [PATCH] qga: Fix handle fd leak in acquire_privilege()
On 05/19/2014 01:26 AM, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com token should be closed in all conditions. So move CloseHandle(token) to out branch. Signed-off-by: Wang Rui moon.wang...@huawei.com Signed-off-by: Gonglei arei.gong...@huawei.com --- qga/commands-win32.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake ebl...@redhat.com -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
