[PATCH] Revert "esx: switch VIR_FREE->g_free in esx*Free*()"
From: Martin Kletzander <mkletzan@redhat.com> This reverts commit 443c79dd7f7d4051fc0084baaa6c56a55d2aace4. 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); - 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); - g_free(*parsedUri); + VIR_FREE((*parsedUri)->transport); + VIR_FREE((*parsedUri)->vCenter); + VIR_FREE((*parsedUri)->proxy_hostname); + VIR_FREE((*parsedUri)->path); + VIR_FREE((*parsedUri)->cacert); + + VIR_FREE(*parsedUri); } -- 2.54.0
On Thu, May 14, 2026 at 03:56:49PM +0200, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
This reverts commit 443c79dd7f7d4051fc0084baaa6c56a55d2aace4.
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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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); }
On Fri, May 15, 2026 at 03:06:26PM -0400, Laine Stump wrote:
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.
I did not check whether the issue started at this point, it might have been that the double-free happened way layer after some other changes, it's just that this particular commit was ideal to revert.
(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).
They are not needed, but it reads better when they are not mixed, and this is not a code that would run each 10ms or something. Not to mention the latency of vmware soap api. But if you feel like changing it so that anything under *p is just g_free()'d and *p VIR_FREE()'d, I'd be completely fine with that.
On 5/18/26 4:15 AM, Martin Kletzander via Devel wrote:
On Fri, May 15, 2026 at 03:06:26PM -0400, Laine Stump wrote:
On 5/14/26 9:56 AM, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
I did not check whether the issue started at this point, it might have been that the double-free happened way layer after some other changes, it's just that this particular commit was ideal to revert.
Since I felt responsible, I did check, and it looks like there was immediately an issue as soon as I made that change. So yeah, it was all me :-/.
(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).
They are not needed, but it reads better when they are not mixed, and this is not a code that would run each 10ms or something. Not to mention the latency of vmware soap api. But if you feel like changing it so that anything under *p is just g_free()'d and *p VIR_FREE()'d, I'd be completely fine with that.
Definitely keep it the way you have it. My prose was a bit obtuse, but I'm almost totally serious about replacing *all* g_free() *everywhere* with VIR_FREE() just for consistency and the tiny chance that it could eliminate (or at least expose) more as-yet-unseen use-after-free cases.
participants (3)
-
Laine Stump -
Martin Kletzander -
Pavel Hrdina