Re: Buildbot failure in Wildebeest Builder on whole buildset
On Sun, Sep 12, 2021 at 11:16:09PM +, build...@builder.wildebeest.org wrote: > The Buildbot has detected a new failure on builder elfutils-fedora-s390x > while building elfutils. > Full details are available at: > https://builder.wildebeest.org/buildbot/#builders/10/builds/795 > > Buildbot URL: https://builder.wildebeest.org/buildbot/ > > Worker for this Build: fedora-s390x This is the same failure we saw on fedora-ppc64 and centos-x86_64 yesterday. https://builder.wildebeest.org/buildbot/#/builders/10/builds/795/steps/8/logs/test-suite_log I still don't understand why. In the logs we can see (for the PORT2 server): [Sun Sep 12 22:56:26 2021] (1493056/1493066): recorded buildid=a0a48245eb29786f7b6853df68ab23cb608b344b file=/home/mjw/bb/wildebeest/elfutils-fedora-s390x/build/tests/dwfllines mtime=1631486319 atype=ED But then, 2 seconds later: [Sun Sep 12 22:56:28 2021] (1493056/1493388): searching for buildid=a0a48245eb29786f7b6853df68ab23cb608b344b artifacttype=debuginfo suffix= [Sun Sep 12 22:56:28 2021] (1493056/1493388): not found [Sun Sep 12 22:56:28 2021] (1493056/1493388): 127.0.0.1:47886 UA:elfutils/0.185,Linux/s390x,fedora/34 XFF: GET /buildid/a0a48245eb29786f7b6853df68ab23cb608b344b/debuginfo 404 9 0+2ms Somewhere inbetween the buildid seems to have been forgotten. But I cannot figure out why or where. It is clearly non-deterministic since normally the tests PASS. Cheers, Mark
Re: [Bug debuginfod/28034] client-side %-escape url characters
Hello,
On Sun, Sep 12, 2021 at 1:24 PM Mark Wielaard wrote:
> The escaped_string is created outside the loop and reused each time
> (good). But...
> > +
> >/* Initialize each handle. */
> >for (int i = 0; i < num_urls; i++)
> > {
> > @@ -904,16 +908,23 @@ debuginfod_query_server (debuginfod_client *c,
> >if (filename) /* must start with / */
> > {
> >/* PR28034 escape characters in completed url to %hh format. */
> > - char *escaped_string;
> > - escaped_string = curl_easy_escape(data[i].handle, filename, 0);
> > + char *loc = escaped_string;
> >if (!escaped_string)
> > {
> >rc = -ENOMEM;
> >goto out2;
> > }
>
> This check, and...
>
> > +
> > + size_t escaped_strlen = strlen(escaped_string);
> > + while ((loc = strstr(loc, "%2F")))
> > +{
> > +loc[0] = '/';
> > +// pull the string back after replacement
> > +memmove(loc+1, loc+3,escaped_strlen - (loc -
> > escaped_string + 2) );
> > +escaped_strlen -=2;
> > +}
>
> the manipulation of the escaped_string, could both also be done
> outside the loop, since they always do the same thing.
Fixed.
>
> I think you should simply add a new ChangeLog entry at the top instead
> of changing an old existing one. And please do mention the Makefile.am
> (TESTS) and (EXTRA_DIST) addition.
Should be good.
> OK, that is certainly a file name with lots of unexpected characters :)
Gotta be sure :)
If there are any more efficiency changes, ChangeLog changes, or other
changes, don't be afraid to reach out again.
Thanks,
Noah Sanci
From fa9104c7435503d08308e18d4e1a82792f83fc56 Mon Sep 17 00:00:00 2001
From: Noah Sanci
Date: Thu, 9 Sep 2021 13:10:33 -0400
Subject: [PATCH] debuginfod: PR28034 - Percent escape debuginfod urls
When requesting some source files, some URL-inconvenient chars
sometimes pop up, including but not limited to '+', '^', and '&'.
Example from f33 libstdc++:
/buildid/44d8485cb75512c2ca5c8f70afbd475cae30af4f/source/usr/src/debug
/gcc-10.3.1-1.fc33.x86_64/obj-x86_64-redhat-linux/x86_64-redhat-linux/
libstdc++-v3/src/c++11/../../../../../libstdc++-v3/src/c++11/
condition_variable.cc
As this URL is passed into debuginfod's handler_cb, it appears that the
+ signs are helpfully unescaped to spaces by libmicrohttpd, which
'course breaks everything.
To combat this, before querying the debuginfod daemon, clients now %
escape the source filename. This converts many alphanumeric characters
into their %-code format, including '/' to %2F. We want to preserve the
'/' in the url, so after conversion replace %2Fs with a '/'.
https://sourceware.org/bugzilla/show_bug.cgi?id=28034
Signed-off-by: Noah Sanci
---
debuginfod/ChangeLog | 4 ++
debuginfod/debuginfod-client.c | 29 +
tests/ChangeLog| 12 +++---
tests/Makefile.am | 2 +
tests/run-debuginfod-percent-escape.sh | 60 ++
5 files changed, 94 insertions(+), 13 deletions(-)
create mode 100755 tests/run-debuginfod-percent-escape.sh
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 1173f9cd..b8eaa88c 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -2,6 +2,10 @@
* debuginfod.cxx (libarchive_fdcache::lookup): Add endl after
obatched(clog) line.
+2021-09-13 Noah Sanci
+
+ * debuginfod-client.c (debuginfod_query_server): Removed constant
+ operations from a loop. curl_free memory.
2021-09-06 Dmitry V. Levin
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index d41723ce..f55e1489 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -883,6 +883,26 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
+ char *escaped_string = NULL;
+ size_t escaped_strlen = 0;
+ if (filename)
+{
+ escaped_string = curl_easy_escape(&target_handle, filename+1, 0);
+ if (!escaped_string)
+{
+ rc = -ENOMEM;
+ goto out2;
+}
+ char *loc = escaped_string;
+ escaped_strlen = strlen(escaped_string) - 2 + (size_t)escaped_string;
+ while ((loc = strstr(loc, "%2F")))
+{
+ loc[0] = '/';
+ // pull the string back after replacement
+ memmove(loc+1, loc+3,escaped_strlen - (size_t)loc );
+ escaped_strlen -=2;
+}
+}
/* Initialize each handle. */
for (int i = 0; i < num_urls; i++)
{
@@ -904,16 +924,8 @@ debuginfod_query_server (debuginfod_client *c,
if (filename) /* must start with / */
{
/* PR28034 escape characters in completed url to %hh format. */
- char *escaped_string;
- escaped_string = curl_easy_escape(data[i].handle, filename, 0);
- if (!escaped_string)
-{
-
Re: [Bug debuginfod/28034] client-side %-escape url characters
Hello
Quick arithmetic change to the original patch with an updated commit message.
Best,
Noah Sanci
From cb2a3957d017f40e1edb35ed8f8fd3b9dee1c6be Mon Sep 17 00:00:00 2001
From: Noah Sanci
Date: Thu, 9 Sep 2021 13:10:33 -0400
Subject: [PATCH] debuginfod: PR28034 - No longer escape '/', and loop
efficiency
Previously, urls containing '/', so most urls, would escape '/' to %2F,
which is undesirable for use in other libraries which may escape
differently. This patch escapes the '/' and replaces all of them
ensuring there are no %2Fs sent.
Some inefficiencies within the code were fixed, such as changing constant
operations of a while loop within a for loop to a while loop outside of
a for loop. Also strlen is no longer used within the loop, simplifying
the interior operations to mere arithmetic.
https://sourceware.org/bugzilla/show_bug.cgi?id=28034
Signed-off-by: Noah Sanci
---
debuginfod/ChangeLog | 4 ++
debuginfod/debuginfod-client.c | 35 +++
tests/ChangeLog| 12 +++---
tests/Makefile.am | 2 +
tests/run-debuginfod-percent-escape.sh | 60 ++
5 files changed, 100 insertions(+), 13 deletions(-)
create mode 100755 tests/run-debuginfod-percent-escape.sh
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 1173f9cd..b8eaa88c 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -2,6 +2,10 @@
* debuginfod.cxx (libarchive_fdcache::lookup): Add endl after
obatched(clog) line.
+2021-09-13 Noah Sanci
+
+ * debuginfod-client.c (debuginfod_query_server): Removed constant
+ operations from a loop. curl_free memory.
2021-09-06 Dmitry V. Levin
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index d41723ce..8a1c68d5 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -883,6 +883,32 @@ debuginfod_query_server (debuginfod_client *c,
data[i].errbuf[0] = '\0';
}
+ char *escaped_string = NULL;
+ size_t escaped_strlen = 0;
+ if (filename)
+{
+ escaped_string = curl_easy_escape(&target_handle, filename+1, 0);
+ if (!escaped_string)
+{
+ rc = -ENOMEM;
+ goto out2;
+}
+ char *loc = escaped_string;
+ escaped_strlen = strlen(escaped_string);
+ while ((loc = strstr(loc, "%2F")))
+{
+ loc[0] = '/';
+ //pull the string back after replacement
+ // loc-escaped_string finds the distance from the origin to the new location
+ // - 2 accounts for the 2F which remain and don't need to be measured.
+ // The two above subtracted from escaped_strlen yields the remaining characters
+ // in the string which we want to pull back
+ memmove(loc+1, loc+3,escaped_strlen - (loc-escaped_string) - 2);
+ //Because the 2F was overwritten in the memmove (as desired) escaped_strlen is
+ // now two shorter.
+ escaped_strlen -= 2;
+}
+}
/* Initialize each handle. */
for (int i = 0; i < num_urls; i++)
{
@@ -904,16 +930,8 @@ debuginfod_query_server (debuginfod_client *c,
if (filename) /* must start with / */
{
/* PR28034 escape characters in completed url to %hh format. */
- char *escaped_string;
- escaped_string = curl_easy_escape(data[i].handle, filename, 0);
- if (!escaped_string)
-{
- rc = -ENOMEM;
- goto out2;
-}
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", server_url, build_id_bytes, type);
@@ -953,6 +971,7 @@ debuginfod_query_server (debuginfod_client *c,
curl_multi_add_handle(curlm, data[i].handle);
}
+ if (filename) curl_free(escaped_string);
/* Query servers in parallel. */
if (vfd >= 0)
dprintf (vfd, "query %d urls in parallel\n", num_urls);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 1154686a..3f219320 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -30,6 +30,11 @@
* run-debuginfod-federation-metrics.sh: Likewise.
* run-debuginfod-federation-sqlite.sh: Likewise.
+2021-09-13 Noah Sanci
+
+ * Makefile.am: added run-debuginfod-percent-escape.sh to TESTS and
+ EXTRA_DIST.
+
2021-09-06 Dmitry V. Levin
* elfcopy.c (copy_elf): Remove cast of malloc return value.
@@ -164,11 +169,8 @@
2021-07-16 Noah Sanci
PR28034
- * run-debuginfod-find.sh: Added a test ensuring files with %
- escapable characters in their paths are accessible. The test
- itself is changing the name of a binary known previously as prog to
- p+r%o$g. General operations such as accessing p+r%o$g acts as the
- test for %-escape checking.
+ * run-debuginfod-percent-escape.sh: Added a test ensuring files with %
+ escapable
Re: [Bug debuginfod/27277] Describe retrieved files when verbose
Hello,
On Sun, Sep 12, 2021 at 3:08 PM Mark Wielaard wrote:
> > run-debuginfod-fd-prefetch-caches.sh was updated so that it doesn't
> > trip and fail as previously greping for a value that should yield zero
> > caused an error.
>
> I think this part should be in this patch.
Do you mean should or shouldn't? Removed for now.
> > Previously, target_handle was used to gather CURLE_OK reponses. Under
> > some conditions, target_handle was NULL when we wanted it to point to
> > the handle. This could cause some failuers. instead msg->easy_handle
> > is used, which ensures the correct handle is used.
>
> Thanks for including this explanation. What were the "some conditions"?
I removed this. For some time there was a some failures related to
target_handle being null, but msg->easy_handle being assigned.
My tests are passing like this, however
> I found https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html
> Maybe you could add that as comment for future readers.
Good idea, added.
> If the stuct handle_data had also a size field then most of the above
> recalculations of the size are unnecessary and since we then know the
> (old) end of response_data we can simply memcpy the new data to the
> end (of course we need to make sure to add a zero terminator, but that
> can be done with one byte wrote instead of doing a memset of the whole
> buffer).
Changed.
> > #if LIBCURL_VERSION_NUM >= 0x072a00 /* 7.42.0 */
> >curl_easy_setopt(data[i].handle, CURLOPT_PATH_AS_IS, (long) 1);
> > #else
> > @@ -961,6 +994,7 @@ debuginfod_query_server (debuginfod_client *c,
> >int committed_to = -1;
> >bool verbose_reported = false;
> >struct timespec start_time, cur_time;
> > + c->winning_headers = NULL;
> >if ( maxtime > 0 && clock_gettime(CLOCK_MONOTONIC_RAW, &start_time) ==
> > -1)
> > {
> >rc = errno;
> > @@ -995,8 +1029,18 @@ debuginfod_query_server (debuginfod_client *c,
> > if (data[i].handle != target_handle)
> > curl_multi_remove_handle(curlm, data[i].handle);
> > else
> > - committed_to = i;
> > - }
> > + {
> > + committed_to = i;
> > +if (c->winning_headers == NULL)
>
> The indenting here is off because of the mixing of spaces and tabs.
Fixed.
> > diff --git a/tests/ChangeLog b/tests/ChangeLog
> > index 1abe5456..23aeec4a 100644
> > --- a/tests/ChangeLog
> > +++ b/tests/ChangeLog
> > @@ -106,11 +106,12 @@
> > run-debuginfod-query-retry.sh,
> > run-debuginfod-regex.sh,
> > run-debuginfod-sizetime.sh,
> > - run-debuginfod-tmp-home.sh, and
> > - run-debuginfod-writable.sh
> > - run-debuginfod-x-forwarded-for.sh
> > - * tests/Makefile.am: Added the above new files to the test suite
> > - * tests/test-subr.sh: Added some general functions for above tests
> > + run-debuginfod-tmp-home.sh,
> > + run-debuginfod-writable.sh, and
> > + run-debuginfod-x-forwarded-for.sh.
> > + All of the above functions now use a 'base' variable to avoid races
> > + * Makefile.am: Added the above new files to the test suite
> > + * test-subr.sh: Added some general functions for above tests
>
> These changes seem unrelated to this patch.
Restored their states.
> > +2021-08-02 Noah Sanci
> > +
> > + PR27277
> > + * run-debuginfod-response-headers.sh: Add a test to ensure that file
> > descriptions
> > + are accurate for files outside or within archives.
> > + * Makefile.am: Add run-debuginfod-response-headers.sh to TESTS.
>
> It also needs to be added to EXTRA_DIST.
Added.
> > diff --git a/tests/run-debuginfod-fd-prefetch-caches.sh
> > b/tests/run-debuginfod-fd-prefetch-caches.sh
> > index 61fee9e9..bee88c1e 100755
> > --- a/tests/run-debuginfod-fd-prefetch-caches.sh
> > +++ b/tests/run-debuginfod-fd-prefetch-caches.sh
> > @@ -51,9 +51,9 @@ grep 'prefetch fds ' vlog$PORT1 #$PREFETCH_FDS
> > grep 'prefetch mbs ' vlog$PORT1 #$PREFETCH_MBS
> > # search the vlog to find what metric counts should be and check the
> > correct metrics
> > # were incrimented
> > -wait_ready $PORT1 'fdcache_op_count{op="enqueue"}' $( grep -c
> > 'interned.*front=1' vlog$PORT1 )
> > -wait_ready $PORT1 'fdcache_op_count{op="evict"}' $( grep -c 'evicted a=.*'
> > vlog$PORT1 )
> > -wait_ready $PORT1 'fdcache_op_count{op="prefetch_enqueue"}' $( grep -c
> > 'interned.*front=0' vlog$PORT1 )
> > +wait_ready $PORT1 'fdcache_op_count{op="enqueue"}' $( grep -c
> > 'interned.*front=1' vlog$PORT1 || true)
> > +wait_ready $PORT1 'fdcache_op_count{op="evict"}' $( grep -c 'evicted a=.*'
> > vlog$PORT1 || true )
> > +wait_ready $PORT1 'fdcache_op_count{op="prefetch_enqueue"}' $( grep -c
> > 'interned.*front=0' vlog$PORT1 || true )
> > wait_ready $PORT1 'fdcache_op_count{op="prefetch_evict"}' $( grep -c
> > 'evicted from prefetch a=.*front=0' vlog$PORT1 || true )
> >
> > kill $PID1
>
> This is an unrelated change.
Restored.
> > diff --git a/tests/run-debuginfod-response
