diff mbox series

[4/5] gqa-win: get_pci_info: Replace 'while' with 2 calls of the function

Message ID 20211213111554.62394-5-konstantin@daynix.com (mailing list archive)
State New, archived
Headers show
Series gqa-win: get_pci_info: Fix memory leak | expand

Commit Message

Konstantin Kostiuk Dec. 13, 2021, 11:15 a.m. UTC
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(-)

--
2.25.1

Comments

Marc-André Lureau Dec. 13, 2021, 7:45 p.m. UTC | #1
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
>
diff mbox series

Patch

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, ...)