nodeDeviceUpdateMediatedDevices invokes virMdevctlListDefined and
virMdevctlListActive both of which were passed the same 'errmsg' buffer.

Since virCommandSetErrorBuffer() always allocates the error buffer one
of them was leaked.

Fix it by populating the 'errmsg' buffer only on failure of
virMdevctlListActive|Defined which invoke the command.

Add a comment to nodeDeviceGetMdevctlListCommand reminding how
virCommandSetErrorBuffer() works.

Fixes: 44a0f2f0c8f
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/node_device/node_device_driver.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index a2d0600560..f8fae71194 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1044,6 +1044,15 @@ virMdevctlSetAutostart(virNodeDeviceDef *def, bool autostart, char **errmsg)
 }


+/**
+ * nodeDeviceGetMdevctlListCommand:
+ * @defined: list mdevctl entries with persistent config
+ * @output: filled with the output of mdevctl once invoked
+ * @errmsg: always allocated, optionally filled with error from 'mdevctl'
+ *
+ * Prepares a virCommand structure to invoke 'mdevctl' caller is responsible to
+ * free the buffers which are filled by the virCommand infrastructure.
+ */
 virCommand*
 nodeDeviceGetMdevctlListCommand(bool defined,
                                 char **output,
@@ -1623,9 +1632,11 @@ virMdevctlListDefined(virNodeDeviceDef ***devs, char **errmsg)
 {
     int status;
     g_autofree char *output = NULL;
-    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output, errmsg);
+    g_autofree char *errbuf = NULL;
+    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output, &errbuf);

     if (virCommandRun(cmd, &status) < 0 || status != 0) {
+        *errmsg = g_steal_pointer(&errbuf);
         return -1;
     }

@@ -1641,9 +1652,11 @@ virMdevctlListActive(virNodeDeviceDef ***devs, char **errmsg)
 {
     int status;
     g_autofree char *output = NULL;
-    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false, &output, errmsg);
+    g_autofree char *errbuf = NULL;
+    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(false, &output, &errbuf);

     if (virCommandRun(cmd, &status) < 0 || status != 0) {
+        *errmsg = g_steal_pointer(&errbuf);
         return -1;
     }

-- 
2.41.0

On 7/19/23 3:41 PM, Peter Krempa wrote:
> nodeDeviceUpdateMediatedDevices invokes virMdevctlListDefined and
> virMdevctlListActive both of which were passed the same 'errmsg' buffer.
> 
> Since virCommandSetErrorBuffer() always allocates the error buffer one
> of them was leaked.
> 
> Fix it by populating the 'errmsg' buffer only on failure of
> virMdevctlListActive|Defined which invoke the command.
> 
> Add a comment to nodeDeviceGetMdevctlListCommand reminding how
> virCommandSetErrorBuffer() works.
> 
> Fixes: 44a0f2f0c8f
> Signed-off-by: Peter Krempa<pkrempa@redhat.com>

LGTM Thanks for catching and fixing it.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294