[PATCH v2] network: fix memory leak in leaseshelper.c
This was triggered in my experiments with the `virsh net-*` command family. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> --- src/network/leaseshelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index e1c0e81828..6383e08c1d 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -83,7 +83,7 @@ main(int argc, char **argv) g_autofree char *custom_lease_file = NULL; const char *ip = NULL; const char *mac = NULL; - const char *leases_str = NULL; + g_autofree const char *leases_str = NULL; const char *iaid = getenv("DNSMASQ_IAID"); const char *clientid = getenv("DNSMASQ_CLIENT_ID"); const char *interface = getenv("DNSMASQ_INTERFACE"); -- 2.51.2
On 1/7/26 15:13, Philipp Schuster wrote:
This was triggered in my experiments with the `virsh net-*` command family.
On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> --- src/network/leaseshelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index e1c0e81828..6383e08c1d 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -83,7 +83,7 @@ main(int argc, char **argv) g_autofree char *custom_lease_file = NULL; const char *ip = NULL; const char *mac = NULL; - const char *leases_str = NULL; + g_autofree const char *leases_str = NULL;
Since it's dynamically allocated it's not 'const' really :-)
const char *iaid = getenv("DNSMASQ_IAID"); const char *clientid = getenv("DNSMASQ_CLIENT_ID"); const char *interface = getenv("DNSMASQ_INTERFACE");
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Michal
On Thu, Jan 08, 2026 at 09:47:39AM +0100, Michal Prívozník via Devel wrote:
On 1/7/26 15:13, Philipp Schuster wrote:
This was triggered in my experiments with the `virsh net-*` command family.
On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> --- src/network/leaseshelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index e1c0e81828..6383e08c1d 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -83,7 +83,7 @@ main(int argc, char **argv) g_autofree char *custom_lease_file = NULL; const char *ip = NULL; const char *mac = NULL; - const char *leases_str = NULL; + g_autofree const char *leases_str = NULL;
Since it's dynamically allocated it's not 'const' really :-)
We should probably add a syntax-check rule to complain about "g_auto*" used at same time as "const" as that's conceptually flawed. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/8/26 10:28, Daniel P. Berrangé wrote:
On Thu, Jan 08, 2026 at 09:47:39AM +0100, Michal Prívozník via Devel wrote:
On 1/7/26 15:13, Philipp Schuster wrote:
This was triggered in my experiments with the `virsh net-*` command family.
On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> --- src/network/leaseshelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index e1c0e81828..6383e08c1d 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -83,7 +83,7 @@ main(int argc, char **argv) g_autofree char *custom_lease_file = NULL; const char *ip = NULL; const char *mac = NULL; - const char *leases_str = NULL; + g_autofree const char *leases_str = NULL;
Since it's dynamically allocated it's not 'const' really :-)
We should probably add a syntax-check rule to complain about "g_auto*" used at same time as "const" as that's conceptually flawed.
Except, g_autofree const char **arr is okay. But if the regex is written well then sure, let's add a syntax-check rule. Michal
Hi there and a happy new year! I have a question regarding your comments. Basic C types (pointers and const qualifiers) are read from right to left, with the exception that the the first two components can be exchanged if one is `const`. Hence: char *: mutable pointer to mutable chars const char *: mutable pointer to const chars (read-only view into any string) char const*: same as ^ const char * const: const pointer to const chars (read-only view into a fixed string) const char * const: same as ^ So I'm reading `g_autofree const char *leases_str = NULL;` as: `leases_str` offers read-only view into any string, and frees what is behind this pointer once it goes out of scope. So how can `g_autofree const char *leases_str = NULL;` be a problem? I'd only see `g_autofree const char * const leases_str = NULL;` to be problematic, which we clearly do not have here. My C may be a bit..rusty and I'm not familiar in libvirt coding conventions, so I might be missing something?
On 1/8/26 14:39, philipp.schuster@cyberus-technology.de wrote:
Hi there and a happy new year! I have a question regarding your comments. Basic C types (pointers and const qualifiers) are read from right to left, with the exception that the the first two components can be exchanged if one is `const`. Hence:
char *: mutable pointer to mutable chars const char *: mutable pointer to const chars (read-only view into any string) char const*: same as ^ const char * const: const pointer to const chars (read-only view into a fixed string) const char * const: same as ^
So I'm reading `g_autofree const char *leases_str = NULL;` as:
`leases_str` offers read-only view into any string, and frees what is behind this pointer once it goes out of scope.
So how can `g_autofree const char *leases_str = NULL;` be a problem? I'd only see `g_autofree const char * const leases_str = NULL;` to be problematic, which we clearly do not have here.
My C may be a bit..rusty and I'm not familiar in libvirt coding conventions, so I might be missing something?
It's not your C skills. But having 'char *' often means the function is also the owner of allocated memory, where as 'const char *' means the function is given an immutable reference. It doesn't map 1:1 into Rust, sorry. In Rust you can clearly distinguish mutability and data ownership on a type level (that's why rustc can reason about your code). It's not that clear in C. Here's the code we're talking about: int main(...) { g_autofree char *leases_str = NULL; ... if (!(leases_str = virJSONValueToString(leases_array_new, true))) { ... } if (virFileRewriteStr(custom_lease_file, 0644, leases_str) < 0) goto cleanup; ... } Here, virJSONValueToString() allocates a new string and returns it to the caller, making it also the owner of it. The only way not change the string is, well, not touch it. Okay, perhaps the following might work too: g_autofree char *leases_str = virJSONValueToString(..); const char *leases_str_const = leases_str; // use leases_str_const only but you see how convoluted that is. Back in the day, when I was at university I also heard an explanation that compiler is free to place 'const' data into a RO memory (which we don't really have, unless you're on an embed system). Michal
On Thu, Jan 08, 2026 at 04:32:34PM +0100, Michal Prívozník via Devel wrote:
On 1/8/26 14:39, philipp.schuster@cyberus-technology.de wrote:
Hi there and a happy new year! I have a question regarding your comments. Basic C types (pointers and const qualifiers) are read from right to left, with the exception that the the first two components can be exchanged if one is `const`. Hence:
char *: mutable pointer to mutable chars const char *: mutable pointer to const chars (read-only view into any string) char const*: same as ^ const char * const: const pointer to const chars (read-only view into a fixed string) const char * const: same as ^
So I'm reading `g_autofree const char *leases_str = NULL;` as:
`leases_str` offers read-only view into any string, and frees what is behind this pointer once it goes out of scope.
So how can `g_autofree const char *leases_str = NULL;` be a problem? I'd only see `g_autofree const char * const leases_str = NULL;` to be problematic, which we clearly do not have here.
My C may be a bit..rusty and I'm not familiar in libvirt coding conventions, so I might be missing something?
It's not your C skills. But having 'char *' often means the function is also the owner of allocated memory, where as 'const char *' means the function is given an immutable reference. It doesn't map 1:1 into Rust, sorry. In Rust you can clearly distinguish mutability and data ownership on a type level (that's why rustc can reason about your code). It's not that clear in C.
An easier way to rationalize this is to consider that g_autofree essentially gets turned back into the manual free() pattern by the compiler ie you write g_autofree const char *lease_str = NULL; ... and it gets effectively turned back into const char *lease_str = NULL; ... g_free(leases_str); and if you tried to compile that you'd get a.c:11:8: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 11 | free(f) | ^ In file included from a.c:2: /usr/include/stdlib.h:687:25: note: expected ‘void *’ but argument is of type ‘const char *’ 687 | extern void free (void *__ptr) __THROW; | ~~~~~~^~~~~ a.c:11:10: error: expected ‘;’ before ‘__attribute__’ 11 | free(f) | ^ it is unfortunate the the compiler can't warn about use of 'const' with attribute(cleanup), but we should still imagine that it should, hence my suggestion to add a syntax-check to fill the gap. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé -
Michal Prívozník -
Philipp Schuster -
philipp.schuster＠cyberus-technology.de