On 5/14/26 9:56 AM, Martin Kletzander via Devel wrote:
From: Martin Kletzander <[email protected]>
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 <[email protected]>
---
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);
}
