Hi Noah,
On Mon, 2021-07-26 at 12:29 -0400, Noah Sanci via Elfutils-devel wrote:
> The realloc returning NULL issue has been resolved and the patch
> successfully rebased onto master. Please find these improvements
> attached.
This looks really good, but I found some small issues.
First it turns out reallocarray isn't available on some older systems.
This is easy to workaround though since it is a fairly simple function
we could provide ourselves if it isn't there. The attached patch does
that. I'll push it if it looks good.
Second there is a small memory leak found by valgrind. We only clean up
the server_url_list on failure, but we should also clean it up when we
are done on success:
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
client.c
index 9d049d33..0f65f115 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1191,6 +1191,9 @@ debuginfod_query_server (debuginfod_client *c,
}
free (data);
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
free (server_urls);
/* don't close fd - we're returning it */
Finally on some systems, but not all, one of the tests in run-
debuginfod-find.sh failed. In particular this one:
# check out the debuginfod logs for the new style status lines
# cat vlog$PORT2
grep -q 'UA:.*XFF:.*GET /buildid/.* 200 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /metrics 200 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /badapi 503 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /buildid/deadbeef.* 404 ' vlog$PORT2
That last one changed error message from 404 to 503. This seems related
to the setting of DEBUGINFOD_URLS earlier by the new test you added. If
I change that to:
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index c2c3b9c3..ec639a38 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -367,8 +367,7 @@ wait_ready $PORT1 'found_sourcerefs_total{source=".rpm
archive"}' $sourcefiles
########################################################################
# PR27983 ensure no duplicate urls are used in when querying servers for files
rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
-export DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1
http://127.0.0.1:$PORT1 http:127.0.0.1:7999"
-env LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/debuginfod/debuginfod-find -v
executable $BUILDID2 > vlog4 2>&1 || true
+env DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1
http://127.0.0.1:$PORT1 http:127.0.0.1:7999" LD_LIBRARY_PATH=$ldpath
${abs_top_builddir}/debuginfod/debuginfod-find -v executable $BUILDID2 > vlog4
2>&1 || true
tempfiles vlog4
if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ];
then
echo "Duplicated servers remain";
Everything seems to pass everywhere.
run-debuginfod-find.sh is really convoluted and we really shouldn't add
more and more interacting tests to it. But if we do maybe we should use
the env DEBUGINFOD_URLS=... trick to minimize changing other tests in
the same file.
If you agree with the above two changes, could you resubmit the patch
with those?
Thanks,
Mark
From 61210f91afa4084a46bd0f84d12aa6d3f29c1271 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@klomp.org>
Date: Wed, 28 Jul 2021 16:46:36 +0200
Subject: [PATCH] lib: Add static inline reallocarray fallback function
Signed-off-by: Mark Wielaard <m...@klomp.org>
---
ChangeLog | 4 ++++
configure.ac | 3 +++
lib/ChangeLog | 4 ++++
lib/system.h | 14 ++++++++++++++
4 files changed, 25 insertions(+)
diff --git a/ChangeLog b/ChangeLog
index a01f6f9f..12b8f403 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2021-07-28 Mark Wielaard <m...@klomp.org>
+
+ * configure.ac (AC_CHECK_DECLS): Add reallocarray check.
+
2021-05-22 Mark Wielaard <m...@klomp.org>
* configure.ac (AC_INIT): Set version to 0.185.
diff --git a/configure.ac b/configure.ac
index b348a717..7caff2c5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -425,6 +425,9 @@ AC_CHECK_DECLS([powerof2],[],[],[#include <sys/param.h>])
AC_CHECK_DECLS([mempcpy],[],[],
[#define _GNU_SOURCE
#include <string.h>])
+AC_CHECK_DECLS([reallocarray],[],[],
+ [#define _GNU_SOURCE
+ #include <stdlib.h>])
AC_CHECK_FUNCS([process_vm_readv])
diff --git a/lib/ChangeLog b/lib/ChangeLog
index dd3ebcab..44366fec 100644
--- a/lib/ChangeLog
+++ b/lib/ChangeLog
@@ -1,3 +1,7 @@
+2021-07-28 Mark Wielaard <m...@klomp.org>
+
+ * system.h (reallocarray): New static inline fallback function.
+
2021-04-19 Martin Liska <mli...@suse.cz>
* system.h (startswith): New function.
diff --git a/lib/system.h b/lib/system.h
index cdf18ed7..58d9deee 100644
--- a/lib/system.h
+++ b/lib/system.h
@@ -38,6 +38,7 @@
#include <byteswap.h>
#include <unistd.h>
#include <string.h>
+#include <stdlib.h>
#if __BYTE_ORDER == __LITTLE_ENDIAN
# define LE32(n) (n)
@@ -70,6 +71,19 @@
((void *) ((char *) memcpy (dest, src, n) + (size_t) n))
#endif
+#if !HAVE_DECL_REALLOCARRAY
+static inline void *
+reallocarray (void *ptr, size_t nmemb, size_t size)
+{
+ if (size > 0 && nmemb > SIZE_MAX / size)
+ {
+ errno = ENOMEM;
+ return NULL;
+ }
+ return realloc (ptr, nmemb * size);
+}
+#endif
+
/* Return TRUE if the start of STR matches PREFIX, FALSE otherwise. */
static inline int
--
2.18.4
