[PATCH 0/7] nss: Couple of fixes and small improvements
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
From: Michal Privoznik <mprivozn@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@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
From: Michal Privoznik <mprivozn@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@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
From: Michal Privoznik <mprivozn@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@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
On Thu, May 22, 2025 at 14:25:13 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@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@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.
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@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@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')? Michal
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@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@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.
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@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@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 :|
From: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@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
On Thu, May 22, 2025 at 14:25:14 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@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.
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@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@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.
From: Michal Privoznik <mprivozn@redhat.com> After previous cleanup, some labels were rendered pointless. Drop them. Signed-off-by: Michal Privoznik <mprivozn@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
From: Michal Privoznik <mprivozn@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@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
On a Thursday in 2025, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@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@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); \
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@redhat.com> Jano
From: Michal Privoznik <mprivozn@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@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
On Thu, May 22, 2025 at 14:25:10 +0200, Michal Privoznik wrote:
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(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (6)
-
Daniel P. Berrangé -
Jiri Denemark -
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa