* [Bug debuginfod/27983] ignore duplicate urls
@ 2021-07-09 19:12 Noah Sanci
2021-07-14 16:36 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-09 19:12 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 60 bytes --]
Hello,
Please find the patch for bug 27983 attached.
Noah
[-- Attachment #2: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 6416 bytes --]
From e37f49a0fd5f27907584b19336cd250d825acc98 Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 7 +++++
debuginfod/debuginfod-client.c | 56 +++++++++++++++++++++++++---------
tests/ChangeLog | 5 +++
tests/run-debuginfod-find.sh | 13 ++++++++
4 files changed, 67 insertions(+), 14 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index d9d11737..24ccb8ef 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,10 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Also update the number of urls to the unduplicated number of
+ urls.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f417b40a..88e154d2 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
char *strtok_saveptr;
char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
+ int unduplicated_urls = 0;
/* Initialize each handle. */
for (int i = 0; i < num_urls && server_url != NULL; i++)
@@ -802,34 +803,59 @@ debuginfod_query_server (debuginfod_client *c,
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
- data[i].fd = fd;
- data[i].target_handle = &target_handle;
- data[i].handle = curl_easy_init();
- if (data[i].handle == NULL)
- {
- rc = -ENETUNREACH;
- goto out1;
- }
- data[i].client = c;
-
- /* Build handle url. Tolerate both http://foo:999 and
- http://foo:999/ forms */
char *slashbuildid;
if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
slashbuildid = "buildid";
else
slashbuildid = "/buildid";
+ char *tmp_url = calloc(PATH_MAX, sizeof(char));
if (filename) /* must start with / */
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
+ snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
slashbuildid, build_id_bytes, type, filename);
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
+ snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
slashbuildid, build_id_bytes, type);
+ /* PR 27983: If the url is already set to be used use, skip it */
+ int url_index = -1;
+ for (url_index = 0; url_index < i; ++url_index)
+ {
+ if(strcmp(tmp_url, data[url_index].url) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ // Ensure that next iteration doesn't skip over an index mid-array
+ i--;
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ continue;
+ }
+ else
+ {
+ unduplicated_urls++;
+ strncpy(data[i].url, tmp_url, PATH_MAX);
+ free (tmp_url);
+ }
+
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
+ data[i].fd = fd;
+ data[i].target_handle = &target_handle;
+ data[i].handle = curl_easy_init();
+ if (data[i].handle == NULL)
+ {
+ rc = -ENETUNREACH;
+ goto out1;
+ }
+ data[i].client = c;
curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
if (vfd >= 0)
curl_easy_setopt(data[i].handle, CURLOPT_ERRORBUFFER, data[i].errbuf);
@@ -863,6 +889,8 @@ debuginfod_query_server (debuginfod_client *c,
curl_multi_add_handle(curlm, data[i].handle);
server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
+ num_urls = unduplicated_urls;
+ data = reallocarray( (void *) data, num_urls, sizeof(struct handle_data));
/* Query servers in parallel. */
if (vfd >= 0)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index b65cbeb7..5747d658 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-02 Mark Wielaard <mark@klomp.org>
* run-debuginfo-find.sh: unset VALGRIND_CMD before testing debuginfod
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 74a5ceff..6faaf70b 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -359,6 +359,19 @@ rm -rf extracted
wait_ready $PORT1 'found_sourcerefs_total{source=".rpm archive"}' $sourcefiles
+########################################################################
+# PR27983 ensure no duplicate urls are used in when querying servers for files
+TMP_DEBUG_URLS=$DEBUGINFOD_URLS
+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
+tempfiles vlog4
+if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
+ echo "Duplicated servers undetected";
+ err
+fi
+export DEBUGINFOD_URLS=$TMP_DEBUG_URLS
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-09 19:12 [Bug debuginfod/27983] ignore duplicate urls Noah Sanci
@ 2021-07-14 16:36 ` Mark Wielaard
2021-07-19 13:31 ` Noah Sanci
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2021-07-14 16:36 UTC (permalink / raw)
To: Noah Sanci; +Cc: elfutils-devel
Hi Noah,
On Fri, Jul 09, 2021 at 03:12:18PM -0400, Noah Sanci via Elfutils-devel wrote:
> From e37f49a0fd5f27907584b19336cd250d825acc98 Mon Sep 17 00:00:00 2001
> From: Noah Sanci <nsanci@redhat.com>
> Date: Fri, 9 Jul 2021 14:53:10 -0400
> Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
>
> Gazing at server logs, one sees a minority of clients who appear to have
> duplicate query traffic coming in: the same URL, milliseconds apart.
> Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> and the client library is dutifully asking the servers TWICE. Bug #27863
> reduces the pain on the servers' CPU, but dupe network traffic is still
> being paid. We should reject sending outright duplicate concurrent
> traffic.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=27983
>
> Signed-off-by: Noah Sanci <nsanci@redhat.com>
> ---
> debuginfod/ChangeLog | 7 +++++
> debuginfod/debuginfod-client.c | 56 +++++++++++++++++++++++++---------
> tests/ChangeLog | 5 +++
> tests/run-debuginfod-find.sh | 13 ++++++++
> 4 files changed, 67 insertions(+), 14 deletions(-)
>
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index d9d11737..24ccb8ef 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,10 @@
> +2021-07-09 Noah Sanci <nsanci@redhat.com>
> +
> + * debuginfod-client.c (debuginfod_query_server): As full-length
> + urls are generated with standardized formats, ignore duplicates.
> + Also update the number of urls to the unduplicated number of
> + urls.
You deduplicate the full URLs after they are fully constructed. Would
it make sense to do the deduplication on server_url, maybe even as
part of the Count number of URLs code? That might make the code
simpler. And you can change num_urls upfront.
> + num_urls = unduplicated_urls;
> + data = reallocarray( (void *) data, num_urls, sizeof(struct handle_data));
Maybe this reallocarray is unnecessary. Yes, it might save a little
bit of memory, but you do have to handle reallocarray failure.
Cheers,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-14 16:36 ` Mark Wielaard
@ 2021-07-19 13:31 ` Noah Sanci
2021-07-20 14:42 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-19 13:31 UTC (permalink / raw)
To: Mark Wielaard, elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]
Hello,
On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard <mark@klomp.org> wrote:
> You deduplicate the full URLs after they are fully constructed. Would
> it make sense to do the deduplication on server_url, maybe even as
> part of the Count number of URLs code? That might make the code
> simpler. And you can change num_urls upfront.
Deduplication before fully building the URL would work well, however I
was concerned about the slashbuildid situation. I would need to alter
all urls to either have a trailing '/' or no trailing '/' to ensure
comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
is considered equal. This is possible, but I ultimately decided to
wait until full construction as those issues would have been handled.
I would be glad to make the change if you want.
>
> > + num_urls = unduplicated_urls;
> > + data = reallocarray( (void *) data, num_urls, sizeof(struct handle_data));
>
> Maybe this reallocarray is unnecessary. Yes, it might save a little
> bit of memory, but you do have to handle reallocarray failure.
Good to know. I removed it.
Thanks
Noah
[-- Attachment #2: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 6339 bytes --]
From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 7 +++++
debuginfod/debuginfod-client.c | 55 +++++++++++++++++++++++++---------
tests/ChangeLog | 5 ++++
tests/run-debuginfod-find.sh | 13 ++++++++
4 files changed, 66 insertions(+), 14 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index d9d11737..24ccb8ef 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,10 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Also update the number of urls to the unduplicated number of
+ urls.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f417b40a..a9447cae 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
char *strtok_saveptr;
char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
+ int unduplicated_urls = 0;
/* Initialize each handle. */
for (int i = 0; i < num_urls && server_url != NULL; i++)
@@ -802,34 +803,59 @@ debuginfod_query_server (debuginfod_client *c,
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
- data[i].fd = fd;
- data[i].target_handle = &target_handle;
- data[i].handle = curl_easy_init();
- if (data[i].handle == NULL)
- {
- rc = -ENETUNREACH;
- goto out1;
- }
- data[i].client = c;
-
- /* Build handle url. Tolerate both http://foo:999 and
- http://foo:999/ forms */
char *slashbuildid;
if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
slashbuildid = "buildid";
else
slashbuildid = "/buildid";
+ char *tmp_url = calloc(PATH_MAX, sizeof(char));
if (filename) /* must start with / */
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
+ snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
slashbuildid, build_id_bytes, type, filename);
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
+ snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
slashbuildid, build_id_bytes, type);
+ /* PR 27983: If the url is already set to be used use, skip it */
+ int url_index = -1;
+ for (url_index = 0; url_index < i; ++url_index)
+ {
+ if(strcmp(tmp_url, data[url_index].url) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ // Ensure that next iteration doesn't skip over an index mid-array
+ i--;
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ continue;
+ }
+ else
+ {
+ unduplicated_urls++;
+ strncpy(data[i].url, tmp_url, PATH_MAX);
+ free (tmp_url);
+ }
+
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
+ data[i].fd = fd;
+ data[i].target_handle = &target_handle;
+ data[i].handle = curl_easy_init();
+ if (data[i].handle == NULL)
+ {
+ rc = -ENETUNREACH;
+ goto out1;
+ }
+ data[i].client = c;
curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
if (vfd >= 0)
curl_easy_setopt(data[i].handle, CURLOPT_ERRORBUFFER, data[i].errbuf);
@@ -863,6 +889,7 @@ debuginfod_query_server (debuginfod_client *c,
curl_multi_add_handle(curlm, data[i].handle);
server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
+ num_urls = unduplicated_urls;
/* Query servers in parallel. */
if (vfd >= 0)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index b65cbeb7..5747d658 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-02 Mark Wielaard <mark@klomp.org>
* run-debuginfo-find.sh: unset VALGRIND_CMD before testing debuginfod
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 74a5ceff..6faaf70b 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -359,6 +359,19 @@ rm -rf extracted
wait_ready $PORT1 'found_sourcerefs_total{source=".rpm archive"}' $sourcefiles
+########################################################################
+# PR27983 ensure no duplicate urls are used in when querying servers for files
+TMP_DEBUG_URLS=$DEBUGINFOD_URLS
+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
+tempfiles vlog4
+if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
+ echo "Duplicated servers undetected";
+ err
+fi
+export DEBUGINFOD_URLS=$TMP_DEBUG_URLS
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-19 13:31 ` Noah Sanci
@ 2021-07-20 14:42 ` Mark Wielaard
2021-07-22 16:25 ` Noah Sanci
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2021-07-20 14:42 UTC (permalink / raw)
To: Noah Sanci; +Cc: elfutils-devel
Hi Noah,
On Mon, Jul 19, 2021 at 09:31:17AM -0400, Noah Sanci wrote:
> On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard <mark@klomp.org> wrote:
> > You deduplicate the full URLs after they are fully constructed. Would
> > it make sense to do the deduplication on server_url, maybe even as
> > part of the Count number of URLs code? That might make the code
> > simpler. And you can change num_urls upfront.
> Deduplication before fully building the URL would work well, however I
> was concerned about the slashbuildid situation. I would need to alter
> all urls to either have a trailing '/' or no trailing '/' to ensure
> comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
> is considered equal. This is possible, but I ultimately decided to
> wait until full construction as those issues would have been handled.
> I would be glad to make the change if you want.
I see what you mean, we have:
/* Build handle url. Tolerate both http://foo:999 and
http://foo:999/ forms */
char *slashbuildid;
if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
slashbuildid = "buildid";
else
slashbuildid = "/buildid";
I think the code could become simpler if we did the deduplication of
the server_url list up-front. That also gives you the oppertunity to
make sure all server_urls end with a slash. But if you think that is
too much work for this patch we can do it as a followup patch. You
already included a test, which makes checking such a refactoring
easier (sadly the run-debuginfod-find.sh test is somewhat fragile).
> From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
> From: Noah Sanci <nsanci@redhat.com>
> Date: Fri, 9 Jul 2021 14:53:10 -0400
> Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
>
> Gazing at server logs, one sees a minority of clients who appear to have
> duplicate query traffic coming in: the same URL, milliseconds apart.
> Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> and the client library is dutifully asking the servers TWICE. Bug #27863
> reduces the pain on the servers' CPU, but dupe network traffic is still
> being paid. We should reject sending outright duplicate concurrent
> traffic.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=27983
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index f417b40a..a9447cae 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
>
> char *strtok_saveptr;
> char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> + int unduplicated_urls = 0;
>
> /* Initialize each handle. */
> for (int i = 0; i < num_urls && server_url != NULL; i++)
Maybe this should be:
/* Initialize each handle. */
int i = 0;
while (server_url != NULL)
With an i++ at the end after the data[i] handle has been completely assigned.
See below (*) for why.
> + char *tmp_url = calloc(PATH_MAX, sizeof(char));
This can fail. So you'll have to handle tmp_url == NULL. Otherwise
snprintf will crash.
> if (filename) /* must start with / */
> - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> slashbuildid, build_id_bytes, type, filename);
> else
> - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
> slashbuildid, build_id_bytes, type);
>
> + /* PR 27983: If the url is already set to be used use, skip it */
> + int url_index = -1;
> + for (url_index = 0; url_index < i; ++url_index)
No need to initialize url_index twice. Just declar int url_index; it
will be assigned 0 at the next line.
> + {
> + if(strcmp(tmp_url, data[url_index].url) == 0)
> + {
> + url_index = -1;
> + break;
> + }
> + }
> + if (url_index == -1)
> + {
> + if (vfd >= 0)
> + dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
> + free(tmp_url);
> + // Ensure that next iteration doesn't skip over an index mid-array
> + i--;
> + server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
> + continue;
(*) this i--; confused me a little. Which is why I suggest turning
that for loop into a while loop.
> + }
> + else
> + {
> + unduplicated_urls++;
> + strncpy(data[i].url, tmp_url, PATH_MAX);
Both strings are max PATH_MAX chars, and tmp_url is zero terminated,
so you can use strcpy.
> + rc = -ENETUNREACH;
> + goto out1;
> + }
> + data[i].client = c;
> curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
> if (vfd >= 0)
> curl_easy_setopt(data[i].handle, CURLOPT_ERRORBUFFER, data[i].errbuf);
> @@ -863,6 +889,7 @@ debuginfod_query_server (debuginfod_client *c,
> curl_multi_add_handle(curlm, data[i].handle);
> server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
(*) So here a i++ (so you don't need the i-- above).
> }
> + num_urls = unduplicated_urls;
>
> /* Query servers in parallel. */
> if (vfd >= 0)
Cheers,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-20 14:42 ` Mark Wielaard
@ 2021-07-22 16:25 ` Noah Sanci
2021-07-22 19:22 ` Noah Sanci
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-22 16:25 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 5814 bytes --]
Hello
Please find the updated solution of PR27983 attached.
Noah
Noah
On Tue, Jul 20, 2021 at 10:42 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Noah,
>
> On Mon, Jul 19, 2021 at 09:31:17AM -0400, Noah Sanci wrote:
> > On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard <mark@klomp.org> wrote:
> > > You deduplicate the full URLs after they are fully constructed. Would
> > > it make sense to do the deduplication on server_url, maybe even as
> > > part of the Count number of URLs code? That might make the code
> > > simpler. And you can change num_urls upfront.
> > Deduplication before fully building the URL would work well, however I
> > was concerned about the slashbuildid situation. I would need to alter
> > all urls to either have a trailing '/' or no trailing '/' to ensure
> > comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
> > is considered equal. This is possible, but I ultimately decided to
> > wait until full construction as those issues would have been handled.
> > I would be glad to make the change if you want.
>
> I see what you mean, we have:
>
> /* Build handle url. Tolerate both http://foo:999 and
> http://foo:999/ forms */
> char *slashbuildid;
> if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> slashbuildid = "buildid";
> else
> slashbuildid = "/buildid";
>
> I think the code could become simpler if we did the deduplication of
> the server_url list up-front. That also gives you the oppertunity to
> make sure all server_urls end with a slash. But if you think that is
> too much work for this patch we can do it as a followup patch. You
> already included a test, which makes checking such a refactoring
> easier (sadly the run-debuginfod-find.sh test is somewhat fragile).
>
> > From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
> > From: Noah Sanci <nsanci@redhat.com>
> > Date: Fri, 9 Jul 2021 14:53:10 -0400
> > Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
> >
> > Gazing at server logs, one sees a minority of clients who appear to have
> > duplicate query traffic coming in: the same URL, milliseconds apart.
> > Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> > and the client library is dutifully asking the servers TWICE. Bug #27863
> > reduces the pain on the servers' CPU, but dupe network traffic is still
> > being paid. We should reject sending outright duplicate concurrent
> > traffic.
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=27983
>
>
> > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> > index f417b40a..a9447cae 100644
> > --- a/debuginfod/debuginfod-client.c
> > +++ b/debuginfod/debuginfod-client.c
> > @@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
> >
> > char *strtok_saveptr;
> > char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> > + int unduplicated_urls = 0;
> >
> > /* Initialize each handle. */
> > for (int i = 0; i < num_urls && server_url != NULL; i++)
>
> Maybe this should be:
>
> /* Initialize each handle. */
> int i = 0;
> while (server_url != NULL)
>
> With an i++ at the end after the data[i] handle has been completely assigned.
> See below (*) for why.
>
> > + char *tmp_url = calloc(PATH_MAX, sizeof(char));
>
> This can fail. So you'll have to handle tmp_url == NULL. Otherwise
> snprintf will crash.
>
> > if (filename) /* must start with / */
> > - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > slashbuildid, build_id_bytes, type, filename);
> > else
> > - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> > + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
> > slashbuildid, build_id_bytes, type);
> >
> > + /* PR 27983: If the url is already set to be used use, skip it */
> > + int url_index = -1;
> > + for (url_index = 0; url_index < i; ++url_index)
>
> No need to initialize url_index twice. Just declar int url_index; it
> will be assigned 0 at the next line.
>
> > + {
> > + if(strcmp(tmp_url, data[url_index].url) == 0)
> > + {
> > + url_index = -1;
> > + break;
> > + }
> > + }
> > + if (url_index == -1)
> > + {
> > + if (vfd >= 0)
> > + dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
> > + free(tmp_url);
> > + // Ensure that next iteration doesn't skip over an index mid-array
> > + i--;
> > + server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
> > + continue;
>
> (*) this i--; confused me a little. Which is why I suggest turning
> that for loop into a while loop.
>
> > + }
> > + else
> > + {
> > + unduplicated_urls++;
> > + strncpy(data[i].url, tmp_url, PATH_MAX);
>
> Both strings are max PATH_MAX chars, and tmp_url is zero terminated,
> so you can use strcpy.
>
> > + rc = -ENETUNREACH;
> > + goto out1;
> > + }
> > + data[i].client = c;
> > curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
> > if (vfd >= 0)
> > curl_easy_setopt(data[i].handle, CURLOPT_ERRORBUFFER, data[i].errbuf);
> > @@ -863,6 +889,7 @@ debuginfod_query_server (debuginfod_client *c,
> > curl_multi_add_handle(curlm, data[i].handle);
> > server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
>
> (*) So here a i++ (so you don't need the i-- above).
>
> > }
> > + num_urls = unduplicated_urls;
> >
> > /* Query servers in parallel. */
> > if (vfd >= 0)
>
> Cheers,
>
> Mark
>
[-- Attachment #2: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 8906 bytes --]
From 84118c56954deef811e94d3a188ea9894430f92d Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
The urls are now simply removed upon finding a duplicate after url
construction.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 8 +++
debuginfod/debuginfod-client.c | 97 ++++++++++++++++++++++++----------
tests/ChangeLog | 5 ++
tests/run-debuginfod-find.sh | 13 +++++
4 files changed, 95 insertions(+), 28 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index d9d11737..23544d3f 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,11 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Created out1 and changed out2 error gotos. Updated url creation print
+ statements.
+ (globals): Removed url_delim_char, as it was no longer used.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f417b40a..1a687831 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -152,7 +152,6 @@ static const char *cache_xdg_name = "debuginfod_client";
/* URLs of debuginfods, separated by url_delim. */
static const char *url_delim = " ";
-static const char url_delim_char = ' ';
/* Timeout for debuginfods, in seconds (to get at least 100K). */
static const long default_timeout = 90;
@@ -763,13 +762,61 @@ debuginfod_query_server (debuginfod_client *c,
goto out0;
}
+
+ /* Initialize the memory to zero */
+ char *strtok_saveptr;
+ char **server_url_list = NULL;
+ char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Count number of URLs. */
int num_urls = 0;
- for (int i = 0; server_urls[i] != '\0'; i++)
- if (server_urls[i] != url_delim_char
- && (i == 0 || server_urls[i - 1] == url_delim_char))
- num_urls++;
-
+
+ while (server_url != NULL)
+ {
+ /* PR 27983: If the url is already set to be used use, skip it */
+ char *slashbuildid;
+ if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
+ slashbuildid = "buildid";
+ else
+ slashbuildid = "/buildid";
+
+ char *tmp_url = calloc(PATH_MAX, sizeof(char));
+ snprintf(tmp_url, PATH_MAX, "%s%s", server_url,
+ slashbuildid);
+ int url_index;
+ for (url_index = 0; url_index < num_urls; ++url_index)
+ {
+ if(strcmp(tmp_url, server_url_list[url_index]) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ }
+ else
+ {
+ num_urls++;
+ server_url_list = reallocarray(server_url_list, num_urls,
+ sizeof(char*));
+ if (server_url_list == NULL)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ server_url_list[num_urls-1] = strdup(tmp_url);
+ if (server_url_list[num_urls-1] == NULL)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ free (tmp_url);
+ }
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ }
CURLM *curlm = c->server_mhandle;
assert (curlm != NULL);
@@ -793,12 +840,11 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
- char *strtok_saveptr;
- char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
-
/* Initialize each handle. */
- for (int i = 0; i < num_urls && server_url != NULL; i++)
+ for (int i = 0; i < num_urls; i++)
{
+ if ((server_url = server_url_list[i]) == NULL)
+ break;
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
@@ -808,25 +854,16 @@ debuginfod_query_server (debuginfod_client *c,
if (data[i].handle == NULL)
{
rc = -ENETUNREACH;
- goto out1;
+ goto out2;
}
data[i].client = c;
/* Build handle url. Tolerate both http://foo:999 and
http://foo:999/ forms */
- char *slashbuildid;
- if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
- slashbuildid = "buildid";
- else
- slashbuildid = "/buildid";
-
- if (filename) /* must start with / */
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
- slashbuildid, build_id_bytes, type, filename);
+ if (filename)
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s%s", server_url, build_id_bytes, type, filename);
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
- slashbuildid, build_id_bytes, type);
-
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s", server_url, build_id_bytes, type);
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
@@ -861,7 +898,6 @@ debuginfod_query_server (debuginfod_client *c,
curl_easy_setopt(data[i].handle, CURLOPT_HTTPHEADER, c->headers);
curl_multi_add_handle(curlm, data[i].handle);
- server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
/* Query servers in parallel. */
@@ -905,7 +941,7 @@ debuginfod_query_server (debuginfod_client *c,
case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
default: rc = -ENETUNREACH; break;
}
- goto out1;
+ goto out2;
}
if (c->progressfn) /* inform/check progress callback */
@@ -1075,7 +1111,7 @@ debuginfod_query_server (debuginfod_client *c,
}
if (verified_handle == NULL)
- goto out1;
+ goto out2;
if (vfd >= 0)
{
@@ -1104,7 +1140,7 @@ debuginfod_query_server (debuginfod_client *c,
if (rc < 0)
{
rc = -errno;
- goto out1;
+ goto out2;
/* Perhaps we need not give up right away; could retry or something ... */
}
@@ -1127,7 +1163,7 @@ debuginfod_query_server (debuginfod_client *c,
goto out;
/* error exits */
- out1:
+ out2:
/* remove all handles from multi */
for (int i = 0; i < num_urls; i++)
{
@@ -1140,6 +1176,11 @@ debuginfod_query_server (debuginfod_client *c,
(void) rmdir (target_cache_dir); /* nop if not empty */
free(data);
+ out1:
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
+
out0:
free (server_urls);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index b65cbeb7..5747d658 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-02 Mark Wielaard <mark@klomp.org>
* run-debuginfo-find.sh: unset VALGRIND_CMD before testing debuginfod
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 74a5ceff..6faaf70b 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -359,6 +359,19 @@ rm -rf extracted
wait_ready $PORT1 'found_sourcerefs_total{source=".rpm archive"}' $sourcefiles
+########################################################################
+# PR27983 ensure no duplicate urls are used in when querying servers for files
+TMP_DEBUG_URLS=$DEBUGINFOD_URLS
+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
+tempfiles vlog4
+if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
+ echo "Duplicated servers undetected";
+ err
+fi
+export DEBUGINFOD_URLS=$TMP_DEBUG_URLS
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-22 16:25 ` Noah Sanci
@ 2021-07-22 19:22 ` Noah Sanci
2021-07-23 18:34 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-22 19:22 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 6244 bytes --]
Hello,
PR27983 improvements attached
Noah
Noah
On Thu, Jul 22, 2021 at 12:25 PM Noah Sanci <nsanci@redhat.com> wrote:
>
> Hello
>
> Please find the updated solution of PR27983 attached.
>
> Noah
>
>
> Noah
>
> On Tue, Jul 20, 2021 at 10:42 AM Mark Wielaard <mark@klomp.org> wrote:
> >
> > Hi Noah,
> >
> > On Mon, Jul 19, 2021 at 09:31:17AM -0400, Noah Sanci wrote:
> > > On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard <mark@klomp.org> wrote:
> > > > You deduplicate the full URLs after they are fully constructed. Would
> > > > it make sense to do the deduplication on server_url, maybe even as
> > > > part of the Count number of URLs code? That might make the code
> > > > simpler. And you can change num_urls upfront.
> > > Deduplication before fully building the URL would work well, however I
> > > was concerned about the slashbuildid situation. I would need to alter
> > > all urls to either have a trailing '/' or no trailing '/' to ensure
> > > comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
> > > is considered equal. This is possible, but I ultimately decided to
> > > wait until full construction as those issues would have been handled.
> > > I would be glad to make the change if you want.
> >
> > I see what you mean, we have:
> >
> > /* Build handle url. Tolerate both http://foo:999 and
> > http://foo:999/ forms */
> > char *slashbuildid;
> > if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> > slashbuildid = "buildid";
> > else
> > slashbuildid = "/buildid";
> >
> > I think the code could become simpler if we did the deduplication of
> > the server_url list up-front. That also gives you the oppertunity to
> > make sure all server_urls end with a slash. But if you think that is
> > too much work for this patch we can do it as a followup patch. You
> > already included a test, which makes checking such a refactoring
> > easier (sadly the run-debuginfod-find.sh test is somewhat fragile).
> >
> > > From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
> > > From: Noah Sanci <nsanci@redhat.com>
> > > Date: Fri, 9 Jul 2021 14:53:10 -0400
> > > Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
> > >
> > > Gazing at server logs, one sees a minority of clients who appear to have
> > > duplicate query traffic coming in: the same URL, milliseconds apart.
> > > Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> > > and the client library is dutifully asking the servers TWICE. Bug #27863
> > > reduces the pain on the servers' CPU, but dupe network traffic is still
> > > being paid. We should reject sending outright duplicate concurrent
> > > traffic.
> > >
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=27983
> >
> >
> > > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> > > index f417b40a..a9447cae 100644
> > > --- a/debuginfod/debuginfod-client.c
> > > +++ b/debuginfod/debuginfod-client.c
> > > @@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
> > >
> > > char *strtok_saveptr;
> > > char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> > > + int unduplicated_urls = 0;
> > >
> > > /* Initialize each handle. */
> > > for (int i = 0; i < num_urls && server_url != NULL; i++)
> >
> > Maybe this should be:
> >
> > /* Initialize each handle. */
> > int i = 0;
> > while (server_url != NULL)
> >
> > With an i++ at the end after the data[i] handle has been completely assigned.
> > See below (*) for why.
> >
> > > + char *tmp_url = calloc(PATH_MAX, sizeof(char));
> >
> > This can fail. So you'll have to handle tmp_url == NULL. Otherwise
> > snprintf will crash.
> >
> > > if (filename) /* must start with / */
> > > - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > > + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > > slashbuildid, build_id_bytes, type, filename);
> > > else
> > > - snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> > > + snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
> > > slashbuildid, build_id_bytes, type);
> > >
> > > + /* PR 27983: If the url is already set to be used use, skip it */
> > > + int url_index = -1;
> > > + for (url_index = 0; url_index < i; ++url_index)
> >
> > No need to initialize url_index twice. Just declar int url_index; it
> > will be assigned 0 at the next line.
> >
> > > + {
> > > + if(strcmp(tmp_url, data[url_index].url) == 0)
> > > + {
> > > + url_index = -1;
> > > + break;
> > > + }
> > > + }
> > > + if (url_index == -1)
> > > + {
> > > + if (vfd >= 0)
> > > + dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
> > > + free(tmp_url);
> > > + // Ensure that next iteration doesn't skip over an index mid-array
> > > + i--;
> > > + server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
> > > + continue;
> >
> > (*) this i--; confused me a little. Which is why I suggest turning
> > that for loop into a while loop.
> >
> > > + }
> > > + else
> > > + {
> > > + unduplicated_urls++;
> > > + strncpy(data[i].url, tmp_url, PATH_MAX);
> >
> > Both strings are max PATH_MAX chars, and tmp_url is zero terminated,
> > so you can use strcpy.
> >
> > > + rc = -ENETUNREACH;
> > > + goto out1;
> > > + }
> > > + data[i].client = c;
> > > curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
> > > if (vfd >= 0)
> > > curl_easy_setopt(data[i].handle, CURLOPT_ERRORBUFFER, data[i].errbuf);
> > > @@ -863,6 +889,7 @@ debuginfod_query_server (debuginfod_client *c,
> > > curl_multi_add_handle(curlm, data[i].handle);
> > > server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
> >
> > (*) So here a i++ (so you don't need the i-- above).
> >
> > > }
> > > + num_urls = unduplicated_urls;
> > >
> > > /* Query servers in parallel. */
> > > if (vfd >= 0)
> >
> > Cheers,
> >
> > Mark
> >
[-- Attachment #2: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 8683 bytes --]
From 8016e84c514d96797d3452f07899fad2a6a03f4e Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
The urls are now simply removed upon finding a duplicate after url
construction.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 8 +++
debuginfod/debuginfod-client.c | 95 ++++++++++++++++++++++++----------
tests/ChangeLog | 5 ++
tests/run-debuginfod-find.sh | 11 ++++
4 files changed, 91 insertions(+), 28 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index d9d11737..23544d3f 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,11 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Created out1 and changed out2 error gotos. Updated url creation print
+ statements.
+ (globals): Removed url_delim_char, as it was no longer used.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f417b40a..d0343ed7 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -152,7 +152,6 @@ static const char *cache_xdg_name = "debuginfod_client";
/* URLs of debuginfods, separated by url_delim. */
static const char *url_delim = " ";
-static const char url_delim_char = ' ';
/* Timeout for debuginfods, in seconds (to get at least 100K). */
static const long default_timeout = 90;
@@ -763,13 +762,59 @@ debuginfod_query_server (debuginfod_client *c,
goto out0;
}
+
+ /* Initialize the memory to zero */
+ char *strtok_saveptr;
+ char **server_url_list = NULL;
+ char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Count number of URLs. */
int num_urls = 0;
- for (int i = 0; server_urls[i] != '\0'; i++)
- if (server_urls[i] != url_delim_char
- && (i == 0 || server_urls[i - 1] == url_delim_char))
- num_urls++;
-
+
+ while (server_url != NULL)
+ {
+ /* PR 27983: If the url is already set to be used use, skip it */
+ char *slashbuildid;
+ if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
+ slashbuildid = "buildid";
+ else
+ slashbuildid = "/buildid";
+
+ char *tmp_url;
+ if (asprintf(&tmp_url, "%s%s", server_url, slashbuildid) == -1)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ int url_index;
+ for (url_index = 0; url_index < num_urls; ++url_index)
+ {
+ if(strcmp(tmp_url, server_url_list[url_index]) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ }
+ else
+ {
+ num_urls++;
+ server_url_list = reallocarray(server_url_list, num_urls,
+ sizeof(char*));
+ if (server_url_list == NULL)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ server_url_list[num_urls-1] = tmp_url;
+ }
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ }
+
CURLM *curlm = c->server_mhandle;
assert (curlm != NULL);
@@ -793,12 +838,11 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
- char *strtok_saveptr;
- char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
-
/* Initialize each handle. */
- for (int i = 0; i < num_urls && server_url != NULL; i++)
+ for (int i = 0; i < num_urls; i++)
{
+ if ((server_url = server_url_list[i]) == NULL)
+ break;
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
@@ -808,25 +852,16 @@ debuginfod_query_server (debuginfod_client *c,
if (data[i].handle == NULL)
{
rc = -ENETUNREACH;
- goto out1;
+ goto out2;
}
data[i].client = c;
/* Build handle url. Tolerate both http://foo:999 and
http://foo:999/ forms */
- char *slashbuildid;
- if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
- slashbuildid = "buildid";
- else
- slashbuildid = "/buildid";
-
- if (filename) /* must start with / */
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
- slashbuildid, build_id_bytes, type, filename);
+ if (filename)
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s%s", server_url, build_id_bytes, type, filename);
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
- slashbuildid, build_id_bytes, type);
-
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s", server_url, build_id_bytes, type);
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
@@ -861,7 +896,6 @@ debuginfod_query_server (debuginfod_client *c,
curl_easy_setopt(data[i].handle, CURLOPT_HTTPHEADER, c->headers);
curl_multi_add_handle(curlm, data[i].handle);
- server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
/* Query servers in parallel. */
@@ -905,7 +939,7 @@ debuginfod_query_server (debuginfod_client *c,
case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
default: rc = -ENETUNREACH; break;
}
- goto out1;
+ goto out2;
}
if (c->progressfn) /* inform/check progress callback */
@@ -1075,7 +1109,7 @@ debuginfod_query_server (debuginfod_client *c,
}
if (verified_handle == NULL)
- goto out1;
+ goto out2;
if (vfd >= 0)
{
@@ -1104,7 +1138,7 @@ debuginfod_query_server (debuginfod_client *c,
if (rc < 0)
{
rc = -errno;
- goto out1;
+ goto out2;
/* Perhaps we need not give up right away; could retry or something ... */
}
@@ -1127,7 +1161,7 @@ debuginfod_query_server (debuginfod_client *c,
goto out;
/* error exits */
- out1:
+ out2:
/* remove all handles from multi */
for (int i = 0; i < num_urls; i++)
{
@@ -1140,6 +1174,11 @@ debuginfod_query_server (debuginfod_client *c,
(void) rmdir (target_cache_dir); /* nop if not empty */
free(data);
+ out1:
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
+
out0:
free (server_urls);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index b65cbeb7..5747d658 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-02 Mark Wielaard <mark@klomp.org>
* run-debuginfo-find.sh: unset VALGRIND_CMD before testing debuginfod
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 74a5ceff..f1848550 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -359,6 +359,17 @@ rm -rf extracted
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
+tempfiles vlog4
+if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
+ echo "Duplicated servers undetected";
+ err
+fi
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-22 19:22 ` Noah Sanci
@ 2021-07-23 18:34 ` Mark Wielaard
2021-07-26 16:29 ` Noah Sanci
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2021-07-23 18:34 UTC (permalink / raw)
To: Noah Sanci, elfutils-devel
Hi Noah,
On Thu, 2021-07-22 at 15:22 -0400, Noah Sanci via Elfutils-devel wrote:
> PR27983 improvements attached
Please do mention the improvements you made. It really does help others
see what you thought of. So in this case use asprintf instead of calloc
plus sprintf, and (re)use tmpurl instead of strduping it and freeing.
Both good improvements.
I really like this variant of the patch. Nice work.
There is one possible issue with the reallocarray call (realloc[array]
is terrible that way, sorry). See below.
Also, could you please rebase this patch on top of master, there have
been various changes to debuginfod-client.c and it would be nice to see
how the patch interacts with those.
> + else
> + {
> + num_urls++;
> + server_url_list = reallocarray(server_url_list, num_urls,
> + sizeof(char*));
> + if (server_url_list == NULL)
> + {
> + rc = -ENOMEM;
> + goto out1;
> + }
> + server_url_list[num_urls-1] = tmp_url;
> + }
Note how reallocarray returns NULL on failure.
This means you lost the original server_url_list pointer.
And so when you then do cleanup by going to out1...
> + out1:
> + for (int i = 0; i < num_urls; ++i)
> + free(server_url_list[i]);
> + free(server_url_list);
You will crash because server_url_list is NULL here, so
server_url_list[i] is a bad dereference...
Bleah. Like I said, the realloc(array) interface is really bad :{
To use reallocarray safely you need to use a temp and only set it after
the call succeeds.
char **realloc_ptr = reallocarray (...);
if (realloc_ptr == NULL)
{
rc = -ENOMEM;
goto out1;
}
server_url_list = realloc_ptr;
server_url_list[num_urls-1] = ...;
The rest of the patch looks good.
Thanks,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-23 18:34 ` Mark Wielaard
@ 2021-07-26 16:29 ` Noah Sanci
2021-07-28 14:55 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-26 16:29 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 155 bytes --]
Hello,
The realloc returning NULL issue has been resolved and the patch
successfully rebased onto master. Please find these improvements
attached.
Noah
[-- Attachment #2: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 9350 bytes --]
From a1b4c2b87f3890c7bf2339b3a69e056d487f74d3 Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
The urls are now simply removed upon finding a duplicate after url
construction.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 9 ++++
debuginfod/debuginfod-client.c | 97 ++++++++++++++++++++++++----------
tests/ChangeLog | 6 +++
tests/run-debuginfod-find.sh | 11 ++++
4 files changed, 94 insertions(+), 29 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 7709c24d..81eb56f9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -34,6 +34,15 @@
(parse_opt): Handle 'r' option by setting regex_groom to true.
(groom): Introduce and use reg_include and reg_exclude.
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ PR27983
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Created out1 and changed out2 error gotos. Updated url creation print
+ statements.
+ (globals): Removed url_delim_char, as it was no longer used.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 26ba1891..9d049d33 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -152,7 +152,6 @@ static const char *cache_xdg_name = "debuginfod_client";
/* URLs of debuginfods, separated by url_delim. */
static const char *url_delim = " ";
-static const char url_delim_char = ' ';
/* Timeout for debuginfods, in seconds (to get at least 100K). */
static const long default_timeout = 90;
@@ -765,13 +764,60 @@ debuginfod_query_server (debuginfod_client *c,
goto out0;
}
+ /* Initialize the memory to zero */
+ char *strtok_saveptr;
+ char **server_url_list = NULL;
+ char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Count number of URLs. */
int num_urls = 0;
- for (int i = 0; server_urls[i] != '\0'; i++)
- if (server_urls[i] != url_delim_char
- && (i == 0 || server_urls[i - 1] == url_delim_char))
- num_urls++;
-
+
+ while (server_url != NULL)
+ {
+ /* PR 27983: If the url is already set to be used use, skip it */
+ char *slashbuildid;
+ if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
+ slashbuildid = "buildid";
+ else
+ slashbuildid = "/buildid";
+
+ char *tmp_url;
+ if (asprintf(&tmp_url, "%s%s", server_url, slashbuildid) == -1)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ int url_index;
+ for (url_index = 0; url_index < num_urls; ++url_index)
+ {
+ if(strcmp(tmp_url, server_url_list[url_index]) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ }
+ else
+ {
+ num_urls++;
+ char ** realloc_ptr;
+ realloc_ptr = reallocarray(server_url_list, num_urls,
+ sizeof(char*));
+ if (realloc_ptr == NULL)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ server_url_list = realloc_ptr;
+ server_url_list[num_urls-1] = tmp_url;
+ }
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ }
+
int retry_limit = default_retry_limit;
const char* retry_limit_envvar = getenv(DEBUGINFOD_RETRY_LIMIT_ENV_VAR);
if (retry_limit_envvar != NULL)
@@ -804,12 +850,11 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
- char *strtok_saveptr;
- char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
-
/* Initialize each handle. */
- for (int i = 0; i < num_urls && server_url != NULL; i++)
+ for (int i = 0; i < num_urls; i++)
{
+ if ((server_url = server_url_list[i]) == NULL)
+ break;
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
@@ -819,18 +864,10 @@ debuginfod_query_server (debuginfod_client *c,
if (data[i].handle == NULL)
{
rc = -ENETUNREACH;
- goto out1;
+ goto out2;
}
data[i].client = c;
- /* Build handle url. Tolerate both http://foo:999 and
- http://foo:999/ forms */
- char *slashbuildid;
- if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
- slashbuildid = "buildid";
- else
- slashbuildid = "/buildid";
-
if (filename) /* must start with / */
{
/* PR28034 escape characters in completed url to %hh format. */
@@ -841,14 +878,12 @@ debuginfod_query_server (debuginfod_client *c,
rc = -ENOMEM;
goto out1;
}
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
- slashbuildid, build_id_bytes, type, escaped_string);
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url,
+ build_id_bytes, type, escaped_string);
curl_free(escaped_string);
}
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
- slashbuildid, build_id_bytes, type);
-
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s", server_url, build_id_bytes, type);
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
@@ -883,7 +918,6 @@ debuginfod_query_server (debuginfod_client *c,
curl_easy_setopt(data[i].handle, CURLOPT_HTTPHEADER, c->headers);
curl_multi_add_handle(curlm, data[i].handle);
- server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
/* Query servers in parallel. */
@@ -927,7 +961,7 @@ debuginfod_query_server (debuginfod_client *c,
case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
default: rc = -ENETUNREACH; break;
}
- goto out1;
+ goto out2;
}
if (c->progressfn) /* inform/check progress callback */
@@ -1115,7 +1149,7 @@ debuginfod_query_server (debuginfod_client *c,
goto query_in_parallel;
}
else
- goto out1;
+ goto out2;
}
if (vfd >= 0)
@@ -1145,7 +1179,7 @@ debuginfod_query_server (debuginfod_client *c,
if (rc < 0)
{
rc = -errno;
- goto out1;
+ goto out2;
/* Perhaps we need not give up right away; could retry or something ... */
}
@@ -1168,7 +1202,7 @@ debuginfod_query_server (debuginfod_client *c,
goto out;
/* error exits */
- out1:
+ out2:
/* remove all handles from multi */
for (int i = 0; i < num_urls; i++)
{
@@ -1181,6 +1215,11 @@ debuginfod_query_server (debuginfod_client *c,
(void) rmdir (target_cache_dir); /* nop if not empty */
free(data);
+ out1:
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
+
out0:
free (server_urls);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 3be9ee48..dba750e4 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -30,6 +30,12 @@
* run-debuginfod-find.sh: Added test case for grooming the database
using regexes.
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ PR27983
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-08 Mark Wielaard <mark@klomp.org>
* Makefile.am (EXTRA_DIST): Fix typo testfile-largealign.bz2 was
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 947c1261..c2c3b9c3 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -364,6 +364,17 @@ rm -rf extracted
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
+tempfiles vlog4
+if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
+ echo "Duplicated servers remain";
+ err
+fi
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-26 16:29 ` Noah Sanci
@ 2021-07-28 14:55 ` Mark Wielaard
2021-07-28 16:23 ` Noah Sanci
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2021-07-28 14:55 UTC (permalink / raw)
To: Noah Sanci, elfutils-devel
[-- 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-28 14:55 ` Mark Wielaard
@ 2021-07-28 16:23 ` Noah Sanci
2021-07-29 12:20 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Noah Sanci @ 2021-07-28 16:23 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 3586 bytes --]
Hello
This patch fixes a memory leak and slightly alters the PR27983 test,
isolating where its DEBUGINFO_URLS's duplicates are accessible, which
fixes a case of test failure on some systems.
Noah
On Wed, Jul 28, 2021 at 10:55 AM Mark Wielaard <mark@klomp.org> wrote:
>
> 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-debuginfod-PR27983-ignore-duplicate-urls.patch --]
[-- Type: text/x-patch, Size: 9879 bytes --]
From b9d36857b6b7cf4f2889280119ba01628afe2420 Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
The urls are now simply removed upon finding a duplicate after url
construction.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
debuginfod/ChangeLog | 9 +++
debuginfod/debuginfod-client.c | 104 +++++++++++++++++++++++----------
tests/ChangeLog | 6 ++
tests/run-debuginfod-find.sh | 11 ++++
4 files changed, 99 insertions(+), 31 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 7709c24d..81eb56f9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -34,6 +34,15 @@
(parse_opt): Handle 'r' option by setting regex_groom to true.
(groom): Introduce and use reg_include and reg_exclude.
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ PR27983
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Created out1 and changed out2 error gotos. Updated url creation print
+ statements.
+ (globals): Removed url_delim_char, as it was no longer used.
+
2021-06-18 Mark Wielaard <mark@klomp.org>
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 26ba1891..5afefbfa 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -152,7 +152,6 @@ static const char *cache_xdg_name = "debuginfod_client";
/* URLs of debuginfods, separated by url_delim. */
static const char *url_delim = " ";
-static const char url_delim_char = ' ';
/* Timeout for debuginfods, in seconds (to get at least 100K). */
static const long default_timeout = 90;
@@ -765,13 +764,60 @@ debuginfod_query_server (debuginfod_client *c,
goto out0;
}
+ /* Initialize the memory to zero */
+ char *strtok_saveptr;
+ char **server_url_list = NULL;
+ char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Count number of URLs. */
int num_urls = 0;
- for (int i = 0; server_urls[i] != '\0'; i++)
- if (server_urls[i] != url_delim_char
- && (i == 0 || server_urls[i - 1] == url_delim_char))
- num_urls++;
-
+
+ while (server_url != NULL)
+ {
+ /* PR 27983: If the url is already set to be used use, skip it */
+ char *slashbuildid;
+ if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
+ slashbuildid = "buildid";
+ else
+ slashbuildid = "/buildid";
+
+ char *tmp_url;
+ if (asprintf(&tmp_url, "%s%s", server_url, slashbuildid) == -1)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ int url_index;
+ for (url_index = 0; url_index < num_urls; ++url_index)
+ {
+ if(strcmp(tmp_url, server_url_list[url_index]) == 0)
+ {
+ url_index = -1;
+ break;
+ }
+ }
+ if (url_index == -1)
+ {
+ if (vfd >= 0)
+ dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+ }
+ else
+ {
+ num_urls++;
+ char ** realloc_ptr;
+ realloc_ptr = reallocarray(server_url_list, num_urls,
+ sizeof(char*));
+ if (realloc_ptr == NULL)
+ {
+ rc = -ENOMEM;
+ goto out1;
+ }
+ server_url_list = realloc_ptr;
+ server_url_list[num_urls-1] = tmp_url;
+ }
+ server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
+ }
+
int retry_limit = default_retry_limit;
const char* retry_limit_envvar = getenv(DEBUGINFOD_RETRY_LIMIT_ENV_VAR);
if (retry_limit_envvar != NULL)
@@ -804,12 +850,11 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
- char *strtok_saveptr;
- char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
-
/* Initialize each handle. */
- for (int i = 0; i < num_urls && server_url != NULL; i++)
+ for (int i = 0; i < num_urls; i++)
{
+ if ((server_url = server_url_list[i]) == NULL)
+ break;
if (vfd >= 0)
dprintf (vfd, "init server %d %s\n", i, server_url);
@@ -819,18 +864,10 @@ debuginfod_query_server (debuginfod_client *c,
if (data[i].handle == NULL)
{
rc = -ENETUNREACH;
- goto out1;
+ goto out2;
}
data[i].client = c;
- /* Build handle url. Tolerate both http://foo:999 and
- http://foo:999/ forms */
- char *slashbuildid;
- if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
- slashbuildid = "buildid";
- else
- slashbuildid = "/buildid";
-
if (filename) /* must start with / */
{
/* PR28034 escape characters in completed url to %hh format. */
@@ -841,14 +878,12 @@ debuginfod_query_server (debuginfod_client *c,
rc = -ENOMEM;
goto out1;
}
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
- slashbuildid, build_id_bytes, type, escaped_string);
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url,
+ build_id_bytes, type, escaped_string);
curl_free(escaped_string);
}
else
- snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
- slashbuildid, build_id_bytes, type);
-
+ snprintf(data[i].url, PATH_MAX, "%s/%s/%s", server_url, build_id_bytes, type);
if (vfd >= 0)
dprintf (vfd, "url %d %s\n", i, data[i].url);
@@ -883,7 +918,6 @@ debuginfod_query_server (debuginfod_client *c,
curl_easy_setopt(data[i].handle, CURLOPT_HTTPHEADER, c->headers);
curl_multi_add_handle(curlm, data[i].handle);
- server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
}
/* Query servers in parallel. */
@@ -927,7 +961,7 @@ debuginfod_query_server (debuginfod_client *c,
case CURLM_OUT_OF_MEMORY: rc = -ENOMEM; break;
default: rc = -ENETUNREACH; break;
}
- goto out1;
+ goto out2;
}
if (c->progressfn) /* inform/check progress callback */
@@ -1115,7 +1149,7 @@ debuginfod_query_server (debuginfod_client *c,
goto query_in_parallel;
}
else
- goto out1;
+ goto out2;
}
if (vfd >= 0)
@@ -1145,7 +1179,7 @@ debuginfod_query_server (debuginfod_client *c,
if (rc < 0)
{
rc = -errno;
- goto out1;
+ goto out2;
/* Perhaps we need not give up right away; could retry or something ... */
}
@@ -1155,7 +1189,10 @@ debuginfod_query_server (debuginfod_client *c,
curl_multi_remove_handle(curlm, data[i].handle); /* ok to repeat */
curl_easy_cleanup (data[i].handle);
}
-
+
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
free (data);
free (server_urls);
@@ -1168,7 +1205,7 @@ debuginfod_query_server (debuginfod_client *c,
goto out;
/* error exits */
- out1:
+ out2:
/* remove all handles from multi */
for (int i = 0; i < num_urls; i++)
{
@@ -1181,6 +1218,11 @@ debuginfod_query_server (debuginfod_client *c,
(void) rmdir (target_cache_dir); /* nop if not empty */
free(data);
+ out1:
+ for (int i = 0; i < num_urls; ++i)
+ free(server_url_list[i]);
+ free(server_url_list);
+
out0:
free (server_urls);
@@ -1213,7 +1255,7 @@ debuginfod_query_server (debuginfod_client *c,
free (target_cache_dir);
free (target_cache_path);
free (target_cache_tmppath);
- return rc;
+ return rc;
}
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 3be9ee48..dba750e4 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -30,6 +30,12 @@
* run-debuginfod-find.sh: Added test case for grooming the database
using regexes.
+2021-07-09 Noah Sanci <nsanci@redhat.com>
+
+ PR27983
+ * run-debuginfod-find.sh: Wrote test to ensure duplicate urls are in
+ fact not checked.
+
2021-07-08 Mark Wielaard <mark@klomp.org>
* Makefile.am (EXTRA_DIST): Fix typo testfile-largealign.bz2 was
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 947c1261..44e16242 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -364,6 +364,17 @@ rm -rf extracted
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
+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";
+ err
+fi
+########################################################################
# Run a bank of queries against the debuginfod-rpms / debuginfod-debs test cases
archive_test() {
--
2.31.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Bug debuginfod/27983] ignore duplicate urls
2021-07-28 16:23 ` Noah Sanci
@ 2021-07-29 12:20 ` Mark Wielaard
0 siblings, 0 replies; 11+ messages in thread
From: Mark Wielaard @ 2021-07-29 12:20 UTC (permalink / raw)
To: Noah Sanci, elfutils-devel
Hi Noah,
On Wed, 2021-07-28 at 12:23 -0400, Noah Sanci via Elfutils-devel wrote:
> This patch fixes a memory leak and slightly alters the PR27983 test,
> isolating where its DEBUGINFO_URLS's duplicates are accessible, which
> fixes a case of test failure on some systems.
Great. I pushed my patch for reallocarray support on older systems and
this one (with two small whitespace fixes).
Thanks,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-29 12:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 19:12 [Bug debuginfod/27983] ignore duplicate urls 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
2021-07-28 16:23 ` Noah Sanci
2021-07-29 12:20 ` Mark Wielaard
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).