[v1] system/vl: Free allocate memory for pid file name in case realpath() failed

[PATCH] system/vl: Free allocate memory for pid file name in case realpath() failed

Posted by Thomas Huth 2 weeks, 1 day ago

From: Thomas Huth <thuth@redhat.com>

In case realpath() fails, the code returns early in the function
qemu_maybe_daemonize(), without freeing the allocated memory. Add
a g_free() here to fix it.

Fixes: dee2a4d4d2f ("vl: defuse PID file path resolve error")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 system/vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/system/vl.c b/system/vl.c
index d2f4044e5d8..7e2ed898240 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2670,6 +2670,7 @@ static void qemu_maybe_daemonize(const char *pid_file)
                 warn_report("not removing PID file on exit: cannot resolve PID "
                             "file path: %s: %s", pid_file, strerror(errno));
             }
+            g_free(pid_file_realpath);
             return;
         }
 
-- 
2.54.0

Re: [PATCH] system/vl: Free allocate memory for pid file name in case realpath() failed

Posted by Fiona Ebner 1 week, 5 days ago

Am 15.05.26 um 4:39 PM schrieb Thomas Huth:
> From: Thomas Huth <thuth@redhat.com>
> 
> In case realpath() fails, the code returns early in the function
> qemu_maybe_daemonize(), without freeing the allocated memory. Add
> a g_free() here to fix it.
> 
> Fixes: dee2a4d4d2f ("vl: defuse PID file path resolve error")
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Thanks!

> ---
>  system/vl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/system/vl.c b/system/vl.c
> index d2f4044e5d8..7e2ed898240 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2670,6 +2670,7 @@ static void qemu_maybe_daemonize(const char *pid_file)
>                  warn_report("not removing PID file on exit: cannot resolve PID "
>                              "file path: %s: %s", pid_file, strerror(errno));
>              }
> +            g_free(pid_file_realpath);
>              return;
>          }
>

Re: [PATCH] system/vl: Free allocate memory for pid file name in case realpath() failed

Posted by Philippe Mathieu-Daudé 2 weeks ago

On 15/5/26 16:38, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> In case realpath() fails, the code returns early in the function
> qemu_maybe_daemonize(), without freeing the allocated memory. Add
> a g_free() here to fix it.
> 
> Fixes: dee2a4d4d2f ("vl: defuse PID file path resolve error")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   system/vl.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/system/vl.c b/system/vl.c
> index d2f4044e5d8..7e2ed898240 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2670,6 +2670,7 @@ static void qemu_maybe_daemonize(const char *pid_file)
>                   warn_report("not removing PID file on exit: cannot resolve PID "
>                               "file path: %s: %s", pid_file, strerror(errno));
>               }
> +            g_free(pid_file_realpath);
>               return;
>           }
>   

Don't we also need to release the other paths?

  - qemu_unlink_pidfile()
  - pid_file_init()
  - pid_file_cleanup()

Re: [PATCH] system/vl: Free allocate memory for pid file name in case realpath() failed

Posted by Thomas Huth 1 week, 5 days ago

On 16/05/2026 12.13, Philippe Mathieu-Daudé wrote:
> On 15/5/26 16:38, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> In case realpath() fails, the code returns early in the function
>> qemu_maybe_daemonize(), without freeing the allocated memory. Add
>> a g_free() here to fix it.
>>
>> Fixes: dee2a4d4d2f ("vl: defuse PID file path resolve error")
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   system/vl.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index d2f4044e5d8..7e2ed898240 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2670,6 +2670,7 @@ static void qemu_maybe_daemonize(const char *pid_file)
>>                   warn_report("not removing PID file on exit: cannot 
>> resolve PID "
>>                               "file path: %s: %s", pid_file, 
>> strerror(errno));
>>               }
>> +            g_free(pid_file_realpath);
>>               return;
>>           }
> 
> Don't we also need to release the other paths?
> 
>   - qemu_unlink_pidfile()
>   - pid_file_init()
>   - pid_file_cleanup()

Adding it to qemu_unlink_pidfile() makes sense here (though QEMU is 
terminating anyway afterwards, some malloc sanitizers might complain 
otherwise), so I sent a v2 for this.

pid_file_init() and pid_file_cleanup() is in the storage daemon, so this is 
a separate problem (the variable just has the same name there). Not sure 
whether it's worth the effort to free the memory there, too, since the code 
exits afterwards anyway, and the pointer is kept 'til the end?

  Thomas

Re: [PATCH] system/vl: Free allocate memory for pid file name in case realpath() failed

Posted by Philippe Mathieu-Daudé 1 week, 5 days ago

On 18/5/26 13:50, Thomas Huth wrote:
> On 16/05/2026 12.13, Philippe Mathieu-Daudé wrote:
>> On 15/5/26 16:38, Thomas Huth wrote:
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> In case realpath() fails, the code returns early in the function
>>> qemu_maybe_daemonize(), without freeing the allocated memory. Add
>>> a g_free() here to fix it.
>>>
>>> Fixes: dee2a4d4d2f ("vl: defuse PID file path resolve error")
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   system/vl.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index d2f4044e5d8..7e2ed898240 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -2670,6 +2670,7 @@ static void qemu_maybe_daemonize(const char 
>>> *pid_file)
>>>                   warn_report("not removing PID file on exit: cannot 
>>> resolve PID "
>>>                               "file path: %s: %s", pid_file, 
>>> strerror(errno));
>>>               }
>>> +            g_free(pid_file_realpath);
>>>               return;
>>>           }
>>
>> Don't we also need to release the other paths?
>>
>>   - qemu_unlink_pidfile()
>>   - pid_file_init()
>>   - pid_file_cleanup()
> 
> Adding it to qemu_unlink_pidfile() makes sense here (though QEMU is 
> terminating anyway afterwards, some malloc sanitizers might complain 
> otherwise), so I sent a v2 for this.
> 
> pid_file_init() and pid_file_cleanup() is in the storage daemon, so this 
> is a separate problem (the variable just has the same name there). Not 
> sure whether it's worth the effort to free the memory there, too, since 
> the code exits afterwards anyway, and the pointer is kept 'til the end?

OK, fine!

> 
>   Thomas
> 
>