[libvirt] [PATCH 0/5] Couple of NSS fixes
I'm trying to develop gethostbyaddr() and meanwhile I've noticed couple of almost trivial patches. Michal Prívozník (5): nss: Compare addresses iff their family matches nss: Drop needless free() in gethostbyname3() nss: Don't leak @addr in gethostbyname4() libvirt_nss.h: Separate function declarations with an empty line tools: Record NSS dependency on symbols file tools/Makefile.am | 8 ++++++++ tools/nss/libvirt_nss.c | 4 +--- tools/nss/libvirt_nss.h | 2 ++ tools/nss/libvirt_nss_leases.c | 6 ++++-- 4 files changed, 15 insertions(+), 5 deletions(-) -- 2.21.0
When parsing leases file, appendAddr() is called to append parsed tuple (address, expiry time, family) into an array. Whilst doing so, the array is searched for possible duplicate. This is done by comparing each item of the array by passed @family: if @family is AF_INET then the item is viewed as IPv4 address. Similarly, if @family is AF_INET6 then the item is viewed as IPv6 address. This is not exactly right - the array can contain addresses of both families and thus the address family of each item of the array must be considered. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss_leases.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 977e3415f7..e96e260da8 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -116,14 +116,16 @@ appendAddr(const char *name __attribute__((unused)), for (i = 0; i < *ntmpAddress; i++) { if (family == AF_INET) { - if (memcmp((*tmpAddress)[i].addr, + if ((*tmpAddress)[i].af == AF_INET && + memcmp((*tmpAddress)[i].addr, &sa.sin.sin_addr, sizeof(sa.sin.sin_addr)) == 0) { DEBUG("IP address already in the list"); return 0; } } else { - if (memcmp((*tmpAddress)[i].addr, + if ((*tmpAddress)[i].af == AF_INET6 && + memcmp((*tmpAddress)[i].addr, &sa.sin6.sin6_addr, sizeof(sa.sin6.sin6_addr)) == 0) { DEBUG("IP address already in the list"); -- 2.21.0
On Sat, Sep 28, 2019 at 10:05:29PM +0200, Michal Privoznik wrote:
When parsing leases file, appendAddr() is called to append parsed tuple (address, expiry time, family) into an array. Whilst doing so, the array is searched for possible duplicate. This is done by comparing each item of the array by passed @family: if @family is AF_INET then the item is viewed as IPv4 address. Similarly, if @family is AF_INET6 then the item is viewed as IPv6 address. This is not exactly right - the array can contain addresses of both families and thus the address family of each item of the array must be considered.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
The findLease() function allocates @addr array iff no error occurred and at least one satisfactory record was found. Therefore, there is no need to call free() if findLease() failed, or did not find any records as addr == NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 89f1f3fdac..8fe5a6d1cf 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -267,7 +267,6 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, af = AF_INET; if ((r = findLease(name, af, &addr, &naddr, &found, errnop)) < 0) { - free(addr); /* Error occurred. Return immediately. */ if (*errnop == EAGAIN) { *herrnop = TRY_AGAIN; @@ -282,13 +281,11 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, /* NOT found */ *errnop = ESRCH; *herrnop = HOST_NOT_FOUND; - free(addr); return NSS_STATUS_NOTFOUND; } else if (!naddr) { /* Found, but no data */ *errnop = ENXIO; *herrnop = NO_DATA; - free(addr); return NSS_STATUS_UNAVAIL; } -- 2.21.0
On Sat, Sep 28, 2019 at 10:05:30PM +0200, Michal Privoznik wrote:
The findLease() function allocates @addr array iff no error occurred and at least one satisfactory record was found. Therefore, there is no need to call free() if findLease() failed, or did not find any records as addr == NULL.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Similarly to gethostbyname3(), the @addr must be freed on return from the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 8fe5a6d1cf..6e332f7578 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -451,6 +451,7 @@ NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, *herrnop = NETDB_SUCCESS; ret = NSS_STATUS_SUCCESS; cleanup: + free(addr); return ret; } #endif /* HAVE_STRUCT_GAIH_ADDRTUPLE */ -- 2.21.0
On Sat, Sep 28, 2019 at 10:05:31PM +0200, Michal Privoznik wrote:
Similarly to gethostbyname3(), the @addr must be freed on return from the function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
I find it more readable that way. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index ee6c971d3d..95ebafdc71 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -71,10 +71,12 @@ enum nss_status NSS_NAME(gethostbyname2)(const char *name, int af, struct hostent *result, char *buffer, size_t buflen, int *errnop, int *herrnop); + enum nss_status 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); + #ifdef HAVE_STRUCT_GAIH_ADDRTUPLE enum nss_status NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, -- 2.21.0
On Sat, Sep 28, 2019 at 10:05:32PM +0200, Michal Privoznik wrote:
I find it more readable that way.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
If a symbol file for either of NSS modules is changed then subsequent 'make' doesn't regenerate the library, because there is no implicit dependency between the library and symbols file. Put an explicit dependency into the Makefile then. Unfortunately, setting _DEPENDENCIES makes us lose automake's generated dependencies (see src/Makefile.am:592 for details). But fortunately, the only dependency we had was _LIBADD variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/Makefile.am b/tools/Makefile.am index 29fdbfe846..ece70384e6 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -535,6 +535,10 @@ nss_libnss_libvirt_la_LDFLAGS = \ nss_libnss_libvirt_la_LIBADD = \ nss/libnss_libvirt_impl.la +nss_libnss_libvirt_la_DEPENDENCIES = \ + $(nss_libnss_libvirt_la_LIBADD) \ + $(LIBVIRT_NSS_SYMBOL_FILE) + noinst_LTLIBRARIES += nss/libnss_libvirt_guest_impl.la nss_libnss_libvirt_guest_impl_la_SOURCES = \ $(LIBVIRT_NSS_SOURCES) \ @@ -567,6 +571,10 @@ nss_libnss_libvirt_guest_la_LDFLAGS = \ nss_libnss_libvirt_guest_la_LIBADD = \ nss/libnss_libvirt_guest_impl.la +nss_libnss_libvirt_guest_la_DEPENDENCIES = \ + $(nss_libnss_libvirt_guest_la_LIBADD) \ + $(LIBVIRT_GUEST_NSS_SYMBOL_FILE) + lib_LTLIBRARIES = \ nss/libnss_libvirt.la \ nss/libnss_libvirt_guest.la -- 2.21.0
On Sat, Sep 28, 2019 at 10:05:33PM +0200, Michal Privoznik wrote:
If a symbol file for either of NSS modules is changed then subsequent 'make' doesn't regenerate the library, because there is no implicit dependency between the library and symbols file. Put an explicit dependency into the Makefile then. Unfortunately, setting _DEPENDENCIES makes us lose automake's generated dependencies (see src/Makefile.am:592 for details). But fortunately, the only dependency we had was _LIBADD variable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
Michal Privoznik