On 5/14/26 9:56 AM, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
This reverts commit 443c79dd7f7d4051fc0084baaa6c56a55d2aace4.
Sigh. *raises hand in shame* :-/ The commit log message *sounds* reasonable; too bad it wasn't telling the full truth - the pointer *is* a local in all cases, but it doesn't point to memory that was allocated in the scope of the function - it just duplicates a pointer within a higher level struct (in these cases the privateData) that does stick around after the function ends. (Some of the changes didn't need to be reverted (those g_free()ing memory pointed to by a member of an object that is itself being g_free()ed (or now VIR_FREE()ed) in the immediately following code), but on the other hand after seeing this disastrous effect of a "simple cleanup", maybe *all* g_free()s should just be changed to VIR_FREE() anyway - it's currently evenly balanced between g_free() and VIR_FREE() (just under 2000 occurrences each (with about 450 uses of g_clear_pointer combined with a *Free() function or g_free() itself).
Change from VIR_FREE() to g_free meant there is a possible double free when there is an error during parsing because the parsing it done directly into the parsedUri member of the esxPrivate, free'd when it fails and then the caller calls free on it again. Changing back to VIR_FREE() means there is no double free and no crash.
Reproducible easily with `virsh -c esx://l?no_verify=2`.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/esx/esx_driver.c | 2 +- src/esx/esx_stream.c | 4 ++-- src/esx/esx_util.c | 13 +++++++------ 3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 010c62b8e880..6ff0db48ac02 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -63,7 +63,7 @@ esxFreePrivate(esxPrivate **priv) esxUtil_FreeParsedUri(&(*priv)->parsedUri); virObjectUnref((*priv)->caps); virObjectUnref((*priv)->xmlopt); - g_free(*priv); + VIR_FREE(*priv); }
diff --git a/src/esx/esx_stream.c b/src/esx/esx_stream.c index 143b2405ed49..c1dd80806feb 100644 --- a/src/esx/esx_stream.c +++ b/src/esx/esx_stream.c @@ -321,8 +321,8 @@ esxFreeStreamPrivate(esxStreamPrivate **priv) return;
esxVI_CURL_Free(&(*priv)->curl); - g_free((*priv)->backlog);
(this one ^^ could have remained)
- g_free(*priv); + VIR_FREE((*priv)->backlog); + VIR_FREE(*priv); }
static int diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index a6275babd542..1443ec3b9e46 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -165,13 +165,14 @@ esxUtil_FreeParsedUri(esxUtil_ParsedUri **parsedUri) if (!parsedUri || !(*parsedUri)) return;
- g_free((*parsedUri)->transport); - g_free((*parsedUri)->vCenter); - g_free((*parsedUri)->proxy_hostname); - g_free((*parsedUri)->path); - g_free((*parsedUri)->cacert);
(All the above *could* have also remained).
- g_free(*parsedUri);
(once this one ^^ was changed to VIR_FREE.)
+ VIR_FREE((*parsedUri)->transport); + VIR_FREE((*parsedUri)->vCenter); + VIR_FREE((*parsedUri)->proxy_hostname); + VIR_FREE((*parsedUri)->path); + VIR_FREE((*parsedUri)->cacert); + + VIR_FREE(*parsedUri); }