* [RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid
@ 2021-08-09 9:48 Kostiantyn Kostiuk
2021-08-09 9:48 ` [RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kostiantyn Kostiuk @ 2021-08-09 9:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Matt Hines, Michael Roth
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 27baf17d6c..2ad8593b82 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] 6+ messages in thread* [RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label 2021-08-09 9:48 [RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk @ 2021-08-09 9:48 ` Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 4/4] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk 2 siblings, 0 replies; 6+ messages in thread From: Kostiantyn Kostiuk @ 2021-08-09 9:48 UTC (permalink / raw) To: qemu-devel; +Cc: Michael Roth, Matt Hines, Michael Roth To prevent memory leaks, always try to free initialized variables. 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 2ad8593b82..724ce76a0e 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] 6+ messages in thread
* [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables 2021-08-09 9:48 [RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk @ 2021-08-09 9:48 ` Kostiantyn Kostiuk 2021-08-09 10:46 ` Philippe Mathieu-Daudé 2021-08-09 9:48 ` [RFC 4/4] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk 2 siblings, 1 reply; 6+ messages in thread From: Kostiantyn Kostiuk @ 2021-08-09 9:48 UTC (permalink / raw) To: qemu-devel; +Cc: Michael Roth, Matt Hines, Michael Roth Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> --- qga/commands-win32.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 724ce76a0e..a8a601776d 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -539,9 +539,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; HDEVINFO parent_dev_info; SP_DEVINFO_DATA parent_dev_info_data; DWORD j; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables 2021-08-09 9:48 ` [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk @ 2021-08-09 10:46 ` Philippe Mathieu-Daudé 2021-08-10 10:49 ` Konstantin Kostiuk 0 siblings, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-09 10:46 UTC (permalink / raw) To: Kostiantyn Kostiuk, qemu-devel, Marc-André Lureau Cc: Michael Roth, Matt Hines, Michael Roth Hi Kostiantyn, On 8/9/21 11:48 AM, Kostiantyn Kostiuk wrote: > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> I'm not sure what you are trying to do here, fix a memory leak? > --- > qga/commands-win32.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 724ce76a0e..a8a601776d 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -539,9 +539,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; > HDEVINFO parent_dev_info; > SP_DEVINFO_DATA parent_dev_info_data; > DWORD j; > Anyhow this function is confuse. I think it would be easier to review by replacing the while() by 2 calls, as suggested in the documentation: https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila -- >8 -- diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 7bac0c5d422..2188c5dd80d 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -539,7 +539,6 @@ 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; STORAGE_DEVICE_NUMBER sdn; char *parent_dev_id = NULL; HDEVINFO parent_dev_info; @@ -551,25 +550,36 @@ 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 (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { - pdev_iface_detail_data = g_malloc(size); - pdev_iface_detail_data->cbSize = - sizeof(*pdev_iface_detail_data); - } else { - error_setg_win32(errp, GetLastError(), - "failed to get device interfaces"); - goto free_dev_info; - } + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA + pdev_iface_detail_data = NULL; + + /* Get the required buffer size. */ + if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, + NULL, 0, &size, + &dev_info_data) + && GetLastError() != ERROR_INSUFFICIENT_BUFFER) { + error_setg_win32(errp, GetLastError(), + "failed to get device interfaces buffer size"); + goto free_dev_info; + } + + /* Allocate an appropriately sized buffer. */ + pdev_iface_detail_data = g_malloc(size); + pdev_iface_detail_data->cbSize = sizeof(*pdev_iface_detail_data); + + /* Get the interface details. */ + if (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, + pdev_iface_detail_data, + size, &size, + &dev_info_data)) { + error_setg_win32(errp, GetLastError(), + "failed to get device interfaces"); + goto free_dev_info; } 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)) { --- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables 2021-08-09 10:46 ` Philippe Mathieu-Daudé @ 2021-08-10 10:49 ` Konstantin Kostiuk 0 siblings, 0 replies; 6+ messages in thread From: Konstantin Kostiuk @ 2021-08-10 10:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael Roth, Marc-André Lureau, Developers, Michael Roth [-- Attachment #1: Type: text/plain, Size: 5320 bytes --] Hi Philippe, On Mon, Aug 9, 2021 at 1:46 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Kostiantyn, > > On 8/9/21 11:48 AM, Kostiantyn Kostiuk wrote: > > Signed-off-by: Kostiantyn Kostiuk <konstantin@daynix.com> > > I'm not sure what you are trying to do here, fix a memory leak? > Yes. This set of patches fix a memory leak. The leak occurs in the case when a storage device does not have a PCI parent. > > > --- > > qga/commands-win32.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index 724ce76a0e..a8a601776d 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -539,9 +539,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; > > HDEVINFO parent_dev_info; > > SP_DEVINFO_DATA parent_dev_info_data; > > DWORD j; > > > > Anyhow this function is confuse. > > I think it would be easier to review by replacing the while() > by 2 calls, as suggested in the documentation: > > https://docs.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceinterfacedetaila > > Ok. I will refactor these patches. > -- >8 -- > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 7bac0c5d422..2188c5dd80d 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -539,7 +539,6 @@ 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; > STORAGE_DEVICE_NUMBER sdn; > char *parent_dev_id = NULL; > HDEVINFO parent_dev_info; > @@ -551,25 +550,36 @@ 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 (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { > - pdev_iface_detail_data = g_malloc(size); > - pdev_iface_detail_data->cbSize = > - sizeof(*pdev_iface_detail_data); > - } else { > - error_setg_win32(errp, GetLastError(), > - "failed to get device interfaces"); > - goto free_dev_info; > - } > + g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA > + pdev_iface_detail_data = NULL; > + > + /* Get the required buffer size. */ > + if (!SetupDiGetDeviceInterfaceDetail(dev_info, > &dev_iface_data, > + NULL, 0, &size, > + &dev_info_data) > + && GetLastError() != ERROR_INSUFFICIENT_BUFFER) { > + error_setg_win32(errp, GetLastError(), > + "failed to get device interfaces > buffer size"); > + goto free_dev_info; > + } > + > + /* Allocate an appropriately sized buffer. */ > + pdev_iface_detail_data = g_malloc(size); > + pdev_iface_detail_data->cbSize = > sizeof(*pdev_iface_detail_data); > + > + /* Get the interface details. */ > + if (!SetupDiGetDeviceInterfaceDetail(dev_info, > &dev_iface_data, > + pdev_iface_detail_data, > + size, &size, > + &dev_info_data)) { > + error_setg_win32(errp, GetLastError(), > + "failed to get device interfaces"); > + goto free_dev_info; > } > > 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)) { > --- > > [-- Attachment #2: Type: text/html, Size: 7279 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC 4/4] gqa-win: get_pci_info: Free parent_dev_info properly 2021-08-09 9:48 [RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk @ 2021-08-09 9:48 ` Kostiantyn Kostiuk 2 siblings, 0 replies; 6+ messages in thread From: Kostiantyn Kostiuk @ 2021-08-09 9:48 UTC (permalink / raw) To: qemu-devel; +Cc: Michael Roth, Matt Hines, Michael Roth 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 <konstantin@daynix.com> --- qga/commands-win32.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index a8a601776d..520c520cb8 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) g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL; STORAGE_DEVICE_NUMBER sdn; g_autofree char *parent_dev_id = NULL; - HDEVINFO parent_dev_info; SP_DEVINFO_DATA parent_dev_info_data; DWORD j; DWORD size = 0; @@ -745,6 +746,7 @@ static GuestPCIAddress *get_pci_info(int number, Error **errp) } } SetupDiDestroyDeviceInfoList(parent_dev_info); + parent_dev_info = INVALID_HANDLE_VALUE; break; } @@ -752,6 +754,9 @@ cleanup: if (dev_info != INVALID_HANDLE_VALUE) { SetupDiDestroyDeviceInfoList(dev_info); } + if (parent_dev_info != INVALID_HANDLE_VALUE) { + SetupDiDestroyDeviceInfoList(parent_dev_info); + } return pci; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-10 10:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-08-09 9:48 [RFC 1/4] gqa-win: get_pci_info: Clean dev_info if handle is valid Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 2/4] gqa-win: get_pci_info: Use common 'cleanup' label Kostiantyn Kostiuk 2021-08-09 9:48 ` [RFC 3/4] gqa-win: get_pci_info: Add g_autofree for few variables Kostiantyn Kostiuk 2021-08-09 10:46 ` Philippe Mathieu-Daudé 2021-08-10 10:49 ` Konstantin Kostiuk 2021-08-09 9:48 ` [RFC 4/4] gqa-win: get_pci_info: Free parent_dev_info properly Kostiantyn Kostiuk
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).