[PATCH 0/7] nss: Couple of fixes and small improvements

[PATCH v3 0/2] Fix virtio console...

[PATCH] virsh: Do not print...

Michal Privoznik

Thursday, 22 May 2025 Thu, 22 May '25

9:25 p.m.

I've noticed most of these after I've turned on debugging for the NSS plugin: diff --git i/tools/nss/libvirt_nss.h w/tools/nss/libvirt_nss.h index 54a0216013..f9c3136985 100644 --- i/tools/nss/libvirt_nss.h +++ w/tools/nss/libvirt_nss.h @@ -35 +35 @@ -#if 0 +#if 1 Michal Prívozník (7): libvirt_nss_macs: Fix type of @len in findMACsFromJSON() nss: Add missing includes for gai_strerror() nss: Declare g_autofree and g_steal_pointer() macros libvirt_nss: Use automatic memory freeing libvirt_nss: Drop needless cleanup labels libvirt_nss: Allocate buffer in ERROR() dynamically libvirt_nss: Allocate buffer in aiforaf() dynamically build-aux/syntax-check.mk | 2 +- tools/nss/libvirt_nss.c | 52 +++++++++++++--------------------- tools/nss/libvirt_nss.h | 33 +++++++++++++++++++-- tools/nss/libvirt_nss_leases.c | 3 ++ tools/nss/libvirt_nss_macs.c | 2 +- 5 files changed, 55 insertions(+), 37 deletions(-) -- 2.49.0

Show replies by date

Michal Privoznik

Thursday, 22 May Thu, 22 May

9:25 p.m.

New subject: [PATCH 1/7] libvirt_nss_macs: Fix type of @len in findMACsFromJSON()

From: Michal Privoznik <mprivozn(a)redhat.com> Inside of findMACsFromJSON(), the retval of json_object_array_length() is stored in a variable that's type of int. But the function is declared to return size_t: /usr/include/json-c/json_object.h:JSON_EXPORT size_t json_object_array_length(const struct json_object *obj); Fix the type of the local variable. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss_macs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c index c3af9375bc..44544624f3 100644 --- a/tools/nss/libvirt_nss_macs.c +++ b/tools/nss/libvirt_nss_macs.c @@ -46,7 +46,7 @@ findMACsFromJSON(json_object *jobj, size_t *nmacs) { size_t i; - int len; + size_t len; if (!json_object_is_type(jobj, json_type_array)) { ERROR("parsed JSON does not contain the leases array"); -- 2.49.0

Michal Privoznik

9:25 p.m.

New subject: [PATCH 2/7] nss: Add missing includes for gai_strerror()

From: Michal Privoznik <mprivozn(a)redhat.com> There are two places where gai_strerror() is called but neither of them includes all necessary header files as documented in its manpage. Fortunately, both calls occur in ERROR() macro which by default does nothing - hence we don't see any compilation errors. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss.c | 2 ++ tools/nss/libvirt_nss_leases.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 69bf59850e..25e2ec0642 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -35,6 +35,8 @@ #include <errno.h> #include <string.h> #include <time.h> +#include <sys/socket.h> +#include <netdb.h> #if defined(WITH_BSD_NSS) diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 4d68787fb2..6624df2928 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -25,6 +25,9 @@ #include <stdlib.h> #include <stdbool.h> #include <fcntl.h> +#include <sys/types.h> +#include <sys/socket.h> +#include <netdb.h> #include <json.h> -- 2.49.0

Michal Privoznik

9:25 p.m.

New subject: [PATCH 3/7] nss: Declare g_autofree and g_steal_pointer() macros

From: Michal Privoznik <mprivozn(a)redhat.com> While we do not want the nss plugin to link with anything but necessary libs (libc and libjson-c) it can benefit from automatic memory freeing. Instead of inventing macros with new name for them, lets stick with g_autofree and g_steal_pointer() which we are used to from the rest of the code. Borrow and simplify definitions for these macros then. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- build-aux/syntax-check.mk | 2 +- tools/nss/libvirt_nss.h | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 62e1604e94..1303a0ce7e 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1421,7 +1421,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ ^(build-aux/syntax-check\.mk|tests/virfilemock\.c)$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c)$$ + ^(docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.[ch])$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index 5f356618f3..a43731de45 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -29,6 +29,7 @@ #include <sys/socket.h> #include <netinet/in.h> #include <netdb.h> +#include <stdlib.h> #if 0 @@ -62,6 +63,29 @@ do { \ # define NSS_NAME(s) _nss_libvirt_guest_##s##_r #endif +#if !defined(g_autofree) +static inline void +generic_free(void *p) +{ + free(*((void **)p)); +} +# define g_autofree __attribute__((cleanup(generic_free))) +#endif + +#if !defined(g_steal_pointer) +static inline void * +g_steal_pointer(void *p) +{ + void **pp = (void **)p; + void *ptr = *pp; + + *pp = NULL; + return ptr; +} +# define g_steal_pointer(x) (__typeof__(*(x))) g_steal_pointer(x) +#endif + + enum nss_status NSS_NAME(gethostbyname)(const char *name, struct hostent *result, char *buffer, size_t buflen, int *errnop, -- 2.49.0

Peter Krempa

10:19 p.m.

New subject: [PATCH 3/7] nss: Declare g_autofree and g_steal_pointer() macros

On Thu, May 22, 2025 at 14:25:13 +0200, Michal Privoznik via Devel wrote:

...

As noted in the next patch it's a bad idea to name this same as glib macros. This code is supposed to be kept glib free and this makes it seem as if glib was used here.

Michal Prívozník

10:22 p.m.

New subject: [PATCH 3/7] nss: Declare g_autofree and g_steal_pointer() macros

On 5/22/25 15:19, Peter Krempa wrote:

...

> @@ -62,6 +63,29 @@ do { \ > # define NSS_NAME(s) _nss_libvirt_guest_##s##_r > #endif > > +#if !defined(g_autofree) > +static inline void > +generic_free(void *p) > +{ > + free(*((void **)p)); > +} > +# define g_autofree __attribute__((cleanup(generic_free))) > +#endif > + > +#if !defined(g_steal_pointer) > +static inline void * > +g_steal_pointer(void *p) > +{ > + void **pp = (void **)p; > + void *ptr = *pp; > + > + *pp = NULL; > + return ptr; > +} > +# define g_steal_pointer(x) (__typeof__(*(x))) g_steal_pointer(x) As noted in the next patch it's a bad idea to name this same as glib macros. This code is supposed to be kept glib free and this makes it seem as if glib was used here.

I can rename it, sure. But I wanted to keep things consistent. Maybe I can get away with: nss_autofree and nss_steal_pointer (that is replace 'g' with 'nss')? Michal

Peter Krempa

10:27 p.m.

New subject: [PATCH 3/7] nss: Declare g_autofree and g_steal_pointer() macros

On Thu, May 22, 2025 at 15:22:14 +0200, Michal Prívozník wrote:

...

On 5/22/25 15:19, Peter Krempa wrote: > On Thu, May 22, 2025 at 14:25:13 +0200, Michal Privoznik via Devel wrote: >> From: Michal Privoznik <mprivozn(a)redhat.com> >> >> While we do not want the nss plugin to link with anything but >> necessary libs (libc and libjson-c) it can benefit from automatic >> memory freeing. Instead of inventing macros with new name for >> them, lets stick with g_autofree and g_steal_pointer() which we >> are used to from the rest of the code. Borrow and simplify >> definitions for these macros then. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> >> --- >> build-aux/syntax-check.mk | 2 +- >> tools/nss/libvirt_nss.h | 24 ++++++++++++++++++++++++ >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> @@ -62,6 +63,29 @@ do { \ >> # define NSS_NAME(s) _nss_libvirt_guest_##s##_r >> #endif >> >> +#if !defined(g_autofree) >> +static inline void >> +generic_free(void *p) >> +{ >> + free(*((void **)p)); >> +} >> +# define g_autofree __attribute__((cleanup(generic_free))) >> +#endif >> + >> +#if !defined(g_steal_pointer) >> +static inline void * >> +g_steal_pointer(void *p) >> +{ >> + void **pp = (void **)p; >> + void *ptr = *pp; >> + >> + *pp = NULL; >> + return ptr; >> +} >> +# define g_steal_pointer(x) (__typeof__(*(x))) g_steal_pointer(x) > > As noted in the next patch it's a bad idea to name this same as > glib macros. This code is supposed to be kept glib free and this makes > it seem as if glib was used here. > I can rename it, sure. But I wanted to keep things consistent. Maybe I can get away with: nss_autofree and nss_steal_pointer (that is replace 'g' with 'nss')?

Well while I don't think it's a good idea to confuse people into thinking the code is using glib even if we're sure it doesn't (by not including it thus the compiler moaning in any other function use, it seems that the consensus from Jirka, Daniel, and you is that it's preferrable to have consistently named functions. Please thus disregard what I wrote in this thread.

Daniel P. Berrangé

10:23 p.m.

New subject: [PATCH 3/7] nss: Declare g_autofree and g_steal_pointer() macros

On Thu, May 22, 2025 at 03:19:18PM +0200, Peter Krempa via Devel wrote:

...

On Thu, May 22, 2025 at 14:25:13 +0200, Michal Privoznik via Devel wrote: > From: Michal Privoznik <mprivozn(a)redhat.com> > > While we do not want the nss plugin to link with anything but > necessary libs (libc and libjson-c) it can benefit from automatic > memory freeing. Instead of inventing macros with new name for > them, lets stick with g_autofree and g_steal_pointer() which we > are used to from the rest of the code. Borrow and simplify > definitions for these macros then. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> > --- > build-aux/syntax-check.mk | 2 +- > tools/nss/libvirt_nss.h | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk > index 62e1604e94..1303a0ce7e 100644 > --- a/build-aux/syntax-check.mk > +++ b/build-aux/syntax-check.mk > @@ -1421,7 +1421,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ > ^(build-aux/syntax-check\.mk|tests/virfilemock\.c)$$ > > exclude_file_name_regexp--sc_prohibit_raw_allocation = \ > - ^(docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c)$$ > + ^(docs/advanced-tests\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.[ch])$$ > > exclude_file_name_regexp--sc_prohibit_readlink = \ > ^src/(util/virutil|lxc/lxc_container)\.c$$ > diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h > index 5f356618f3..a43731de45 100644 > --- a/tools/nss/libvirt_nss.h > +++ b/tools/nss/libvirt_nss.h > @@ -29,6 +29,7 @@ > #include <sys/socket.h> > #include <netinet/in.h> > #include <netdb.h> > +#include <stdlib.h> > > > #if 0 > @@ -62,6 +63,29 @@ do { \ > # define NSS_NAME(s) _nss_libvirt_guest_##s##_r > #endif > > +#if !defined(g_autofree) > +static inline void > +generic_free(void *p) > +{ > + free(*((void **)p)); > +} > +# define g_autofree __attribute__((cleanup(generic_free))) > +#endif > + > +#if !defined(g_steal_pointer) > +static inline void * > +g_steal_pointer(void *p) > +{ > + void **pp = (void **)p; > + void *ptr = *pp; > + > + *pp = NULL; > + return ptr; > +} > +# define g_steal_pointer(x) (__typeof__(*(x))) g_steal_pointer(x) As noted in the next patch it's a bad idea to name this same as glib macros. This code is supposed to be kept glib free and this makes it seem as if glib was used here.

I take the opposite view - having common terminology for the same concept across the codebase is more important as it reduces the learning ramp for contributors. We avoid accidental use of other parts of glib by simply not having the header file present, so it is pretty quickly identified that you can't use arbitrary glib code if you were to try it. 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 :|

Michal Privoznik

9:25 p.m.

New subject: [PATCH 4/7] libvirt_nss: Use automatic memory freeing

From: Michal Privoznik <mprivozn(a)redhat.com> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 25e2ec0642..8460b63228 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -134,7 +134,7 @@ findLease(const char *name, DEBUG("Dir: %s", leaseDir); while ((entry = readdir(dir)) != NULL) { - char *path; + g_autofree char *path = NULL; size_t dlen = strlen(entry->d_name); if (dlen >= 7 && !strcmp(entry->d_name + dlen - 7, ".status")) { @@ -148,18 +148,15 @@ findLease(const char *name, if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) goto cleanup; - leaseFiles[nleaseFiles++] = path; + leaseFiles[nleaseFiles++] = g_steal_pointer(&path); #if defined(LIBVIRT_NSS_GUEST) } else if (dlen >= 5 && !strcmp(entry->d_name + dlen - 5, ".macs")) { if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) goto cleanup; DEBUG("Processing %s", path); - if (findMACs(path, name, &macs, &nmacs) < 0) { - free(path); + if (findMACs(path, name, &macs, &nmacs) < 0) goto cleanup; - } - free(path); #endif /* LIBVIRT_NSS_GUEST */ } @@ -253,7 +250,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, { enum nss_status ret = NSS_STATUS_UNAVAIL; char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; - leaseAddress *addr = NULL; + g_autofree leaseAddress *addr = NULL; size_t naddr, i; bool found = false; size_t nameLen, need, idx = 0; @@ -359,7 +356,6 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, ret = NSS_STATUS_SUCCESS; cleanup: - free(addr); return ret; } @@ -370,7 +366,7 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, int *herrnop, int32_t *ttlp) { enum nss_status ret = NSS_STATUS_UNAVAIL; - leaseAddress *addr = NULL; + g_autofree leaseAddress *addr = NULL; size_t naddr, i; bool found = false; int r; @@ -453,7 +449,6 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, *herrnop = NETDB_SUCCESS; ret = NSS_STATUS_SUCCESS; cleanup: - free(addr); return ret; } #endif /* WITH_STRUCT_GAIH_ADDRTUPLE */ -- 2.49.0

Peter Krempa

10 p.m.

New subject: [PATCH 4/7] libvirt_nss: Use automatic memory freeing

On Thu, May 22, 2025 at 14:25:14 +0200, Michal Privoznik via Devel wrote:

...

IIRC the nss plugin was meant to be kept free of both the libvirt library and glib as it's actually linked into the address space of other processes.

...

diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 25e2ec0642..8460b63228 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -134,7 +134,7 @@ findLease(const char *name, DEBUG("Dir: %s", leaseDir); while ((entry = readdir(dir)) != NULL) { - char *path; + g_autofree char *path = NULL;

So while we're okay using __attribute__(cleanup) I don't think we can use glib here.

Michal Prívozník

10:09 p.m.

New subject: [PATCH 4/7] libvirt_nss: Use automatic memory freeing

On 5/22/25 15:00, Peter Krempa wrote:

...

On Thu, May 22, 2025 at 14:25:14 +0200, Michal Privoznik via Devel wrote: > From: Michal Privoznik <mprivozn(a)redhat.com> > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> > --- > tools/nss/libvirt_nss.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) IIRC the nss plugin was meant to be kept free of both the libvirt library and glib as it's actually linked into the address space of other processes. > > diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c > index 25e2ec0642..8460b63228 100644 > --- a/tools/nss/libvirt_nss.c > +++ b/tools/nss/libvirt_nss.c > @@ -134,7 +134,7 @@ findLease(const char *name, > > DEBUG("Dir: %s", leaseDir); > while ((entry = readdir(dir)) != NULL) { > - char *path; > + g_autofree char *path = NULL; So while we're okay using __attribute__(cleanup) I don't think we can use glib here.

Please see my previous patch to understand why this is NOT grabbing glib in. Michal

Peter Krempa

10:16 p.m.

New subject: [PATCH 4/7] libvirt_nss: Use automatic memory freeing

On Thu, May 22, 2025 at 15:09:24 +0200, Michal Prívozník wrote:

...

On 5/22/25 15:00, Peter Krempa wrote: > On Thu, May 22, 2025 at 14:25:14 +0200, Michal Privoznik via Devel wrote: >> From: Michal Privoznik <mprivozn(a)redhat.com> >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> >> --- >> tools/nss/libvirt_nss.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) > > IIRC the nss plugin was meant to be kept free of both the libvirt > library and glib as it's actually linked into the address space of other > processes. > >> >> diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c >> index 25e2ec0642..8460b63228 100644 >> --- a/tools/nss/libvirt_nss.c >> +++ b/tools/nss/libvirt_nss.c >> @@ -134,7 +134,7 @@ findLease(const char *name, >> >> DEBUG("Dir: %s", leaseDir); >> while ((entry = readdir(dir)) != NULL) { >> - char *path; >> + g_autofree char *path = NULL; > > So while we're okay using __attribute__(cleanup) I don't think we can > use glib here. > Please see my previous patch to understand why this is NOT grabbing glib in.

I see. Well it's extremely confusing to name the macro the same as in glib. As you can clearly see it makes it seem that glib is used. I suggest you pick a different name.

Michal Privoznik

9:25 p.m.

New subject: [PATCH 5/7] libvirt_nss: Drop needless cleanup labels

From: Michal Privoznik <mprivozn(a)redhat.com> After previous cleanup, some labels were rendered pointless. Drop them. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 8460b63228..c1e51441b2 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -248,7 +248,6 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp, char **canonp) { - enum nss_status ret = NSS_STATUS_UNAVAIL; char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; g_autofree leaseAddress *addr = NULL; size_t naddr, i; @@ -303,8 +302,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, if (buflen < need) { *errnop = ENOMEM; *herrnop = TRY_AGAIN; - ret = NSS_STATUS_TRYAGAIN; - goto cleanup; + return NSS_STATUS_TRYAGAIN; } /* First, append name */ @@ -354,9 +352,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, *herrnop = NETDB_SUCCESS; h_errno = 0; - ret = NSS_STATUS_SUCCESS; - cleanup: - return ret; + return NSS_STATUS_SUCCESS; } #ifdef WITH_STRUCT_GAIH_ADDRTUPLE @@ -365,7 +361,6 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp) { - enum nss_status ret = NSS_STATUS_UNAVAIL; g_autofree leaseAddress *addr = NULL; size_t naddr, i; bool found = false; @@ -408,8 +403,7 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, if (buflen < need) { *errnop = ENOMEM; *herrnop = TRY_AGAIN; - ret = NSS_STATUS_TRYAGAIN; - goto cleanup; + return NSS_STATUS_TRYAGAIN; } /* First, append name */ @@ -447,9 +441,7 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, /* Explicitly reset all error variables */ *errnop = 0; *herrnop = NETDB_SUCCESS; - ret = NSS_STATUS_SUCCESS; - cleanup: - return ret; + return NSS_STATUS_SUCCESS; } #endif /* WITH_STRUCT_GAIH_ADDRTUPLE */ -- 2.49.0

Michal Privoznik

9:25 p.m.

New subject: [PATCH 6/7] libvirt_nss: Allocate buffer in ERROR() dynamically

From: Michal Privoznik <mprivozn(a)redhat.com> So far, inside of the ERROR() macro there's pretty large buffer allocated on the stack (for use by strerror_r()). Problem is, with our current stack size limit of 2048 bytes we may come pretty close to the limit or even overshoot it, e.g. in aiforaf() where the function itself declares another stack allocated buffer 1024 bytes long. Just allocate the buffer dynamically. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index a43731de45..54a0216013 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -35,14 +35,17 @@ #if 0 # include <errno.h> # include <stdio.h> +# include <string.h> # define NULLSTR(s) ((s) ? (s) : "<null>") # define ERROR(...) \ do { \ - char ebuf[512]; \ - const char *errmsg = strerror_r(errno, ebuf, sizeof(ebuf)); \ + int saved_errno = errno; \ + const size_t ebuf_size = 512; \ + g_autofree char *ebuf = calloc(ebuf_size, sizeof(*ebuf)); \ + if (ebuf) strerror_r(saved_errno, ebuf, ebuf_size); \ fprintf(stderr, "ERROR %s:%d : ", __FUNCTION__, __LINE__); \ fprintf(stderr, __VA_ARGS__); \ - fprintf(stderr, " : %s\n", errmsg); \ + fprintf(stderr, " : %s\n", NULLSTR(ebuf)); \ fprintf(stderr, "\n"); \ } while (0) -- 2.49.0

Ján Tomko

10:21 p.m.

New subject: [PATCH 6/7] libvirt_nss: Allocate buffer in ERROR() dynamically

On a Thursday in 2025, Michal Privoznik via Devel wrote:

...

Please put the body of the if on a separate line.

...

fprintf(stderr, "ERROR %s:%d : ", __FUNCTION__, __LINE__); \ fprintf(stderr, __VA_ARGS__); \ - fprintf(stderr, " : %s\n", errmsg); \ + fprintf(stderr, " : %s\n", NULLSTR(ebuf)); \ fprintf(stderr, "\n"); \ } while (0)

Reviewed-by: Ján Tomko <jtomko(a)redhat.com> Jano

Michal Privoznik

9:25 p.m.

New subject: [PATCH 7/7] libvirt_nss: Allocate buffer in aiforaf() dynamically

From: Michal Privoznik <mprivozn(a)redhat.com> While we were trying to decrease stack usage of some functions, in v9.8.0-rc1~217 we introduced a couple of internal blocks to the aiforaf() and declared some variables inside those blocks hoping the compiler will reuse the stack for each block. While in general this might be a good strategy, specifically in case of NSS_NAME(gethostbyname2) this is a terrible thing to do. Problem is, NSS_NAME(gethostbyname2) is given a caller allocated buffer and an address of a pointer where the resolved address is stored. And you've probably guessed it already: upon successful return, the pointer is set to point somewhere inside the buffer. Now, if the buffer doesn't live long enough, which in our case it does not (since it was left in the previous block), we should refrain from dereferencing the resolved pointer. Just allocate the buffer on the heap. Fixes: 9e5f2fe4021ada74adbe34ca03be60812c91f334 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index c1e51441b2..6ee328a8df 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -464,18 +464,17 @@ aiforaf(const char *name, struct hostent resolved; int err; char **addrList; + g_autofree char *buf = NULL; + const size_t buf_size = 1024; + int herr; - /* Note: The do-while blocks in this function are used to scope off large - * stack allocated buffers, which are not needed at the same time */ - do { - char buf[1024] = { 0 }; - int herr; + if (!(buf = calloc(buf_size, sizeof(*buf)))) + return; - if (NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr) != NS_SUCCESS) - return; - } while (false); + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, buf_size, + &err, &herr) != NS_SUCCESS) + return; addrList = resolved.h_addr_list; while (*addrList) { -- 2.49.0

Jiri Denemark

10:12 p.m.

On Thu, May 22, 2025 at 14:25:10 +0200, Michal Privoznik wrote:

...

Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com>

days inactive

days old

devel@lists.libvirt.org

Manage subscription

16 comments

6 participants

tags (0)

participants (6)

Daniel P. Berrangé
Jiri Denemark
Ján Tomko
Michal Privoznik
Michal Prívozník
Peter Krempa

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCH 0/7] nss: Couple of fixes and small improvements