* [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak
@ 2021-12-13 11:15 Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
This set of patches fixes memory leaks in the get_pci_info function when it fails
to get parent device data. In this case the parent_dev_info and the parent_dev_id
variables will be initialized, but not freed.
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1958825
Kostiantyn Kostiuk (5):
gqa-win: get_pci_info: Clean dev_info if handle is valid
gqa-win: get_pci_info: Use common 'cleanup' label
gqa-win: get_pci_info: Free parent_dev_info properly
gqa-win: get_pci_info: Replace 'while' with 2 calls of the function
gqa-win: get_pci_info: Add g_autofree for few variables
qga/commands-win32.c | 74 ++++++++++++++++++++++++++++----------------
1 file changed, 47 insertions(+), 27 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
@ 2021-12-13 11:15 ` Kostiantyn Kostiuk
2021-12-13 19:37 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.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 4e84afd83b..3dd74fe225 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -514,7 +514,7 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
static GuestPCIAddress *get_pci_info(int number, Error **errp)
{
- HDEVINFO dev_info;
+ HDEVINFO dev_info = INVALID_HANDLE_VALUE;
SP_DEVINFO_DATA dev_info_data;
SP_DEVICE_INTERFACE_DATA dev_iface_data;
HANDLE dev_file;
@@ -749,7 +749,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
}
free_dev_info:
- SetupDiDestroyDeviceInfoList(dev_info);
+ if (dev_info != INVALID_HANDLE_VALUE) {
+ SetupDiDestroyDeviceInfoList(dev_info);
+ }
out:
return pci;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
@ 2021-12-13 11:15 ` Kostiantyn Kostiuk
2021-12-13 19:39 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
To prevent memory leaks, always try to free initialized variables.
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
qga/commands-win32.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3dd74fe225..12f7a88078 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -532,7 +532,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
if (dev_info == INVALID_HANDLE_VALUE) {
error_setg_win32(errp, GetLastError(), "failed to get devices tree");
- goto out;
+ goto cleanup;
}
g_debug("enumerating devices");
@@ -562,7 +562,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device interfaces");
- goto free_dev_info;
+ goto cleanup;
}
}
@@ -576,7 +576,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
CloseHandle(dev_file);
error_setg_win32(errp, GetLastError(),
"failed to get device slot number");
- goto free_dev_info;
+ goto cleanup;
}
CloseHandle(dev_file);
@@ -586,7 +586,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device interfaces");
- goto free_dev_info;
+ goto cleanup;
}
g_debug("found device slot %d. Getting storage controller", number);
@@ -603,7 +603,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
} else {
error_setg_win32(errp, GetLastError(),
"failed to get device instance ID");
- goto out;
+ goto cleanup;
}
}
@@ -617,14 +617,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
g_error("CM_Locate_DevInst failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get device instance");
- goto out;
+ goto cleanup;
}
cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
if (cr != CR_SUCCESS) {
g_error("CM_Get_Parent failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device instance");
- goto out;
+ goto cleanup;
}
cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
@@ -632,7 +632,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device ID length");
- goto out;
+ goto cleanup;
}
++dev_id_size;
@@ -647,7 +647,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
g_error("CM_Get_Device_ID failed with code %lx", cr);
error_setg_win32(errp, GetLastError(),
"failed to get parent device ID");
- goto out;
+ goto cleanup;
}
}
@@ -661,14 +661,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
if (parent_dev_info == INVALID_HANDLE_VALUE) {
error_setg_win32(errp, GetLastError(),
"failed to get parent device");
- goto out;
+ goto cleanup;
}
parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) {
error_setg_win32(errp, GetLastError(),
"failed to get parent device data");
- goto out;
+ goto cleanup;
}
for (j = 0;
@@ -748,11 +748,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
break;
}
-free_dev_info:
+cleanup:
if (dev_info != INVALID_HANDLE_VALUE) {
SetupDiDestroyDeviceInfoList(dev_info);
}
-out:
return pci;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk
@ 2021-12-13 11:15 ` Kostiantyn Kostiuk
2021-12-13 19:40 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
4 siblings, 1 reply; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
In case when the function fails to get parent device data,
the parent_dev_info variable will be initialized, but not freed.
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
qga/commands-win32.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 12f7a88078..cef14a8762 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -515,6 +515,8 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
static GuestPCIAddress *get_pci_info(int number, Error **errp)
{
HDEVINFO dev_info = INVALID_HANDLE_VALUE;
+ HDEVINFO parent_dev_info = INVALID_HANDLE_VALUE;
+
SP_DEVINFO_DATA dev_info_data;
SP_DEVICE_INTERFACE_DATA dev_iface_data;
HANDLE dev_file;
@@ -542,7 +544,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
STORAGE_DEVICE_NUMBER sdn;
char *parent_dev_id = NULL;
- HDEVINFO parent_dev_info;
SP_DEVINFO_DATA parent_dev_info_data;
DWORD j;
DWORD size = 0;
@@ -744,11 +745,13 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
break;
}
}
- SetupDiDestroyDeviceInfoList(parent_dev_info);
break;
}
cleanup:
+ if (parent_dev_info != INVALID_HANDLE_VALUE) {
+ SetupDiDestroyDeviceInfoList(parent_dev_info);
+ }
if (dev_info != INVALID_HANDLE_VALUE) {
SetupDiDestroyDeviceInfoList(dev_info);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
` (2 preceding siblings ...)
2021-12-13 11:15 ` [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
@ 2021-12-13 11:15 ` Kostiantyn Kostiuk
2021-12-13 19:45 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
4 siblings, 1 reply; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
Microsoft suggests this solution in the documentation:
https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
qga/commands-win32.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index cef14a8762..6bde5260e8 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -552,10 +552,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data,
&GUID_DEVINTERFACE_DISK, 0,
&dev_iface_data)) {
- while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
- pdev_iface_detail_data,
- size, &size,
- &dev_info_data)) {
+ if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+ pdev_iface_detail_data,
+ size, &size,
+ &dev_info_data)) {
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
pdev_iface_detail_data = g_malloc(size);
pdev_iface_detail_data->cbSize =
@@ -567,6 +567,16 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
}
}
+ if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+ pdev_iface_detail_data,
+ size, &size,
+ &dev_info_data)) {
+ // pdev_iface_detail_data already is allocated
+ error_setg_win32(errp, GetLastError(),
+ "failed to get device interfaces");
+ goto cleanup;
+ }
+
dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
NULL);
@@ -597,8 +607,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
ULONG dev_id_size = 0;
size = 0;
- while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
- parent_dev_id, size, &size)) {
+ if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
+ parent_dev_id, size, &size)) {
if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
parent_dev_id = g_malloc(size);
} else {
@@ -608,6 +618,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
}
}
+ if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
+ parent_dev_id, size, &size)) {
+ // parent_dev_id already is allocated
+ error_setg_win32(errp, GetLastError(),
+ "failed to get device instance ID");
+ goto cleanup;
+ }
+
/*
* CM API used here as opposed to
* SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...)
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
` (3 preceding siblings ...)
2021-12-13 11:15 ` [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
@ 2021-12-13 11:15 ` Kostiantyn Kostiuk
2021-12-13 19:50 ` Marc-André Lureau
4 siblings, 1 reply; 11+ messages in thread
From: Kostiantyn Kostiuk @ 2021-12-13 11:15 UTC (permalink / raw)
To: qemu-devel, Michael Roth, Marc-André Lureau,
Philippe Mathieu-Daudé
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
---
qga/commands-win32.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6bde5260e8..96737f33e1 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -541,9 +541,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
- PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
+ g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
STORAGE_DEVICE_NUMBER sdn;
- char *parent_dev_id = NULL;
+ g_autofree char *parent_dev_id = NULL;
SP_DEVINFO_DATA parent_dev_info_data;
DWORD j;
DWORD size = 0;
@@ -580,7 +580,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
NULL);
- g_free(pdev_iface_detail_data);
if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
@@ -675,7 +674,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
parent_dev_info =
SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id,
NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
- g_free(parent_dev_id);
if (parent_dev_info == INVALID_HANDLE_VALUE) {
error_setg_win32(errp, GetLastError(),
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid
2021-12-13 11:15 ` [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
@ 2021-12-13 19:37 ` Marc-André Lureau
0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-13 19:37 UTC (permalink / raw)
To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel
On Mon, Dec 13, 2021 at 3:16 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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 4e84afd83b..3dd74fe225 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -514,7 +514,7 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
>
> static GuestPCIAddress *get_pci_info(int number, Error **errp)
> {
> - HDEVINFO dev_info;
> + HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> SP_DEVINFO_DATA dev_info_data;
> SP_DEVICE_INTERFACE_DATA dev_iface_data;
> HANDLE dev_file;
> @@ -749,7 +749,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> }
>
> free_dev_info:
> - SetupDiDestroyDeviceInfoList(dev_info);
> + if (dev_info != INVALID_HANDLE_VALUE) {
> + SetupDiDestroyDeviceInfoList(dev_info);
> + }
> out:
> return pci;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label
2021-12-13 11:15 ` [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk
@ 2021-12-13 19:39 ` Marc-André Lureau
0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-13 19:39 UTC (permalink / raw)
To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel
On Mon, Dec 13, 2021 at 3:16 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> To prevent memory leaks, always try to free initialized variables.
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@gmail.com>
(nit: since it's a common return point, I would rather name the label
"end" instead)
> ---
> qga/commands-win32.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3dd74fe225..12f7a88078 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -532,7 +532,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> if (dev_info == INVALID_HANDLE_VALUE) {
> error_setg_win32(errp, GetLastError(), "failed to get devices tree");
> - goto out;
> + goto cleanup;
> }
>
> g_debug("enumerating devices");
> @@ -562,7 +562,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> } else {
> error_setg_win32(errp, GetLastError(),
> "failed to get device interfaces");
> - goto free_dev_info;
> + goto cleanup;
> }
> }
>
> @@ -576,7 +576,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> CloseHandle(dev_file);
> error_setg_win32(errp, GetLastError(),
> "failed to get device slot number");
> - goto free_dev_info;
> + goto cleanup;
> }
>
> CloseHandle(dev_file);
> @@ -586,7 +586,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> } else {
> error_setg_win32(errp, GetLastError(),
> "failed to get device interfaces");
> - goto free_dev_info;
> + goto cleanup;
> }
>
> g_debug("found device slot %d. Getting storage controller", number);
> @@ -603,7 +603,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> } else {
> error_setg_win32(errp, GetLastError(),
> "failed to get device instance ID");
> - goto out;
> + goto cleanup;
> }
> }
>
> @@ -617,14 +617,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> g_error("CM_Locate_DevInst failed with code %lx", cr);
> error_setg_win32(errp, GetLastError(),
> "failed to get device instance");
> - goto out;
> + goto cleanup;
> }
> cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0);
> if (cr != CR_SUCCESS) {
> g_error("CM_Get_Parent failed with code %lx", cr);
> error_setg_win32(errp, GetLastError(),
> "failed to get parent device instance");
> - goto out;
> + goto cleanup;
> }
>
> cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0);
> @@ -632,7 +632,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> g_error("CM_Get_Device_ID_Size failed with code %lx", cr);
> error_setg_win32(errp, GetLastError(),
> "failed to get parent device ID length");
> - goto out;
> + goto cleanup;
> }
>
> ++dev_id_size;
> @@ -647,7 +647,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> g_error("CM_Get_Device_ID failed with code %lx", cr);
> error_setg_win32(errp, GetLastError(),
> "failed to get parent device ID");
> - goto out;
> + goto cleanup;
> }
> }
>
> @@ -661,14 +661,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> if (parent_dev_info == INVALID_HANDLE_VALUE) {
> error_setg_win32(errp, GetLastError(),
> "failed to get parent device");
> - goto out;
> + goto cleanup;
> }
>
> parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) {
> error_setg_win32(errp, GetLastError(),
> "failed to get parent device data");
> - goto out;
> + goto cleanup;
> }
>
> for (j = 0;
> @@ -748,11 +748,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> break;
> }
>
> -free_dev_info:
> +cleanup:
> if (dev_info != INVALID_HANDLE_VALUE) {
> SetupDiDestroyDeviceInfoList(dev_info);
> }
> -out:
> return pci;
> }
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly
2021-12-13 11:15 ` [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
@ 2021-12-13 19:40 ` Marc-André Lureau
0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-13 19:40 UTC (permalink / raw)
To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel
On Mon, Dec 13, 2021 at 3:16 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> In case when the function fails to get parent device data,
> the parent_dev_info variable will be initialized, but not freed.
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/commands-win32.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 12f7a88078..cef14a8762 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -515,6 +515,8 @@ DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT,
> static GuestPCIAddress *get_pci_info(int number, Error **errp)
> {
> HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> + HDEVINFO parent_dev_info = INVALID_HANDLE_VALUE;
> +
> SP_DEVINFO_DATA dev_info_data;
> SP_DEVICE_INTERFACE_DATA dev_iface_data;
> HANDLE dev_file;
> @@ -542,7 +544,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> STORAGE_DEVICE_NUMBER sdn;
> char *parent_dev_id = NULL;
> - HDEVINFO parent_dev_info;
> SP_DEVINFO_DATA parent_dev_info_data;
> DWORD j;
> DWORD size = 0;
> @@ -744,11 +745,13 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> break;
> }
> }
> - SetupDiDestroyDeviceInfoList(parent_dev_info);
> break;
> }
>
> cleanup:
> + if (parent_dev_info != INVALID_HANDLE_VALUE) {
> + SetupDiDestroyDeviceInfoList(parent_dev_info);
> + }
> if (dev_info != INVALID_HANDLE_VALUE) {
> SetupDiDestroyDeviceInfoList(dev_info);
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function
2021-12-13 11:15 ` [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
@ 2021-12-13 19:45 ` Marc-André Lureau
0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-13 19:45 UTC (permalink / raw)
To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel
On Mon, Dec 13, 2021 at 3:16 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Microsoft suggests this solution in the documentation:
> https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
Arguably, the next patch should come first, as you introduce extra
leaking points here - alternatively, update the commit message.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/commands-win32.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index cef14a8762..6bde5260e8 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -552,10 +552,10 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data,
> &GUID_DEVINTERFACE_DISK, 0,
> &dev_iface_data)) {
> - while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> - pdev_iface_detail_data,
> - size, &size,
> - &dev_info_data)) {
> + if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> + pdev_iface_detail_data,
> + size, &size,
> + &dev_info_data)) {
> if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> pdev_iface_detail_data = g_malloc(size);
> pdev_iface_detail_data->cbSize =
> @@ -567,6 +567,16 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> }
> }
>
> + if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> + pdev_iface_detail_data,
> + size, &size,
> + &dev_info_data)) {
> + // pdev_iface_detail_data already is allocated
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device interfaces");
> + goto cleanup;
> + }
> +
> dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
> NULL);
> @@ -597,8 +607,8 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> ULONG dev_id_size = 0;
>
> size = 0;
> - while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> - parent_dev_id, size, &size)) {
> + if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> + parent_dev_id, size, &size)) {
> if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> parent_dev_id = g_malloc(size);
> } else {
> @@ -608,6 +618,14 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> }
> }
>
> + if (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data,
> + parent_dev_id, size, &size)) {
> + // parent_dev_id already is allocated
> + error_setg_win32(errp, GetLastError(),
> + "failed to get device instance ID");
> + goto cleanup;
> + }
> +
> /*
> * CM API used here as opposed to
> * SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables
2021-12-13 11:15 ` [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
@ 2021-12-13 19:50 ` Marc-André Lureau
0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-13 19:50 UTC (permalink / raw)
To: Kostiantyn Kostiuk; +Cc: Michael Roth, Philippe Mathieu-Daudé, qemu-devel
On Mon, Dec 13, 2021 at 3:16 PM Kostiantyn Kostiuk
<konstantin@daynix.com> wrote:
>
> Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
(250loc.. the function would deserve to be refactored to not be so long..)
> ---
> qga/commands-win32.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6bde5260e8..96737f33e1 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -541,9 +541,9 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA);
> dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
> - PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL;
> STORAGE_DEVICE_NUMBER sdn;
> - char *parent_dev_id = NULL;
> + g_autofree char *parent_dev_id = NULL;
> SP_DEVINFO_DATA parent_dev_info_data;
> DWORD j;
> DWORD size = 0;
> @@ -580,7 +580,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> FILE_SHARE_READ, NULL, OPEN_EXISTING, 0,
> NULL);
> - g_free(pdev_iface_detail_data);
>
> if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
> NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
> @@ -675,7 +674,6 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp)
> parent_dev_info =
> SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id,
> NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
> - g_free(parent_dev_id);
>
> if (parent_dev_info == INVALID_HANDLE_VALUE) {
> error_setg_win32(errp, GetLastError(),
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-12-13 19:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 11:15 [PATCH 0/5] gqa-win: get_pci_info: Fix memory leak Kostiantyn Kostiuk
2021-12-13 11:15 ` [PATCH 1/5] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk
2021-12-13 19:37 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 2/5] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk
2021-12-13 19:39 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 3/5] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
2021-12-13 19:40 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function Kostiantyn Kostiuk
2021-12-13 19:45 ` Marc-André Lureau
2021-12-13 11:15 ` [PATCH 5/5] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk
2021-12-13 19:50 ` Marc-André Lureau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.