From: Mark Wielaard <mark@klomp.org>
To: Noah Sanci <nsanci@redhat.com>, elfutils-devel@sourceware.org
Subject: Re: [Bug debuginfod/27983] ignore duplicate urls
Date: Wed, 28 Jul 2021 16:55:00 +0200 [thread overview]
Message-ID: <308d7b3305efae40152abbf3b4c9c06b3d6a22fa.camel@klomp.org> (raw)
In-Reply-To: <CAJXA7qhoaK-CbmBY+7kf0BetQ5F3ORqeCr4oxU3MT9btCx3g7A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]
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
[-- Attachment #2: 0001-lib-Add-static-inline-reallocarray-fallback-function.patch --]
[-- Type: text/x-patch, Size: 2251 bytes --]
From 61210f91afa4084a46bd0f84d12aa6d3f29c1271 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@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 <mark@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 <mark@klomp.org>
+
+ * configure.ac (AC_CHECK_DECLS): Add reallocarray check.
+
2021-05-22 Mark Wielaard <mark@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 <mark@klomp.org>
+
+ * system.h (reallocarray): New static inline fallback function.
+
2021-04-19 Martin Liska <mliska@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
next prev parent reply other threads:[~2021-07-28 14:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-09 19:12 Noah Sanci
2021-07-14 16:36 ` Mark Wielaard
2021-07-19 13:31 ` Noah Sanci
2021-07-20 14:42 ` Mark Wielaard
2021-07-22 16:25 ` Noah Sanci
2021-07-22 19:22 ` Noah Sanci
2021-07-23 18:34 ` Mark Wielaard
2021-07-26 16:29 ` Noah Sanci
2021-07-28 14:55 ` Mark Wielaard [this message]
2021-07-28 16:23 ` Noah Sanci
2021-07-29 12:20 ` Mark Wielaard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=308d7b3305efae40152abbf3b4c9c06b3d6a22fa.camel@klomp.org \
--to=mark@klomp.org \
--cc=elfutils-devel@sourceware.org \
--cc=nsanci@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).