Re: [Bug debuginfod/28034] client-side %-escape url characters
Hi Noah,
On Thu, Sep 09, 2021 at 01:28:21PM -0400, Noah Sanci via Elfutils-devel wrote:
> The attached patch %-escapes debuginfod url characters, then unescapes only
> '/' characters. Previously characters such as '+' were not escaped and caused
> improper escaping further on in handler_cb.
> https://sourceware.org/bugzilla/show_bug.cgi?id=28034.
>
> On Wed, Sep 8, 2021 at 9:38 AM Mark Wielaard wrote:
> > /* Initialize each handle. */
> > for (int i = 0; i < num_urls; i++)
> >
> > So you only need to escape once. You of course then need to make sure
> > the escaped_string is freed after the loop.
> Added
>
> > We already check that the first char is a '/'. It seems silly to curl
> > escape that one and then unescape it again. So maybe curl_easy_escape
> > (data[i].handle, filename + 1, 0) and then change the snprintf pattern
> > to "%s/%s/%s/%s"?
> > ^ the slash got readded here.
> Added
>
> > The strlen inside the while loop can also be done outside and then
> > calculated instead of running strlen on the tail every time.
> Added
Thanks. I do have one more performance nitpick (sorry).
> + char *escaped_string;
> + if (filename)
> +escaped_string = curl_easy_escape(&target_handle, filename+1, 0);
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.
>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 +964,7 @@ debuginfod_query_server (debuginfod_client *c,
>curl_multi_add_handle(curlm, data[i].handle);
> }
>
> + if (filename) curl_free(escaped_string);
And released after the loop. Good.
>/* 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 85dca442..b84f420c 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -132,11 +132,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 characters in their paths are accessible.
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.
> 2021-07-21 Noah Sanci
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c586422e..ee17d3b1 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -231,6 +231,7 @@ TESTS += run-debuginfod-dlopen.sh \
>run-debuginfod-federation-sqlite.sh \
>run-debuginfod-federation-link.sh \
>run-debuginfod-federation-metrics.sh \
> + run-debuginfod-percent-escape.sh \
>run-debuginfod-x-forwarded-for.sh
> endif
> endif
> @@ -515,6 +516,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
>run-debuginfod-archive-groom.sh \
>run-debuginfod-archive-rename.sh \
> run-debuginfod-archive-test.sh \
> + run-debuginfod-percent-escape.sh \
>debuginfod-rpms/fedora30/hello2-1.0-2.src.rpm \
>debuginfod-rpms/fedora30/hello2-1.0-2.x86_64.rpm \
>debuginfod-rpms/fedora30/hello2-debuginfo-1.0-2.x86_64.rpm \
> +env DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
> DEBUGINFOD_URLS="http://127.0.0.1:$PORT1"; \
> +LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/
Re: [Bug debuginfod/27277] Describe retrieved files when verbose
Hi Noah,
On Fri, Sep 10, 2021 at 02:22:00PM -0400, Noah Sanci via Elfutils-devel wrote:
> From 979f19eb4fd7a35ace4ddafed103922559b93120 Mon Sep 17 00:00:00 2001
> From: Noah Sanci
> Date: Wed, 28 Jul 2021 14:46:05 -0400
> Subject: [PATCH 2/2] debuginfod: PR27277 - Describe retrieved files when
> verbose
>
> Allow users, with enough verbosity, to print the HTTP response headers
> upon retrieving a file. These files may include several custome http
> response headers such as X-DEBUGINFOD-FILE, X-DEBUGINFOD-SIZE, and
> X-DEBUGINFOD-ARCHIVE. These headers are added from the daemon, in
> debuginfod.cxx.
> 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.
> 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"?
> +2021-08-02 Noah Sanci
> +
> + PR27277
> + * debuginfod-client.c (struct debuginfod_client): New field
> + winning_headers.
> + (struct handle_data): New field response_data.
> + (header_callback): Store received headers in response_data.
> + (debuginfod_query_server): Activate CURLOPT_HEADERFUNCTION.
> + Save winning response_data.
> + (debuginfod_end): free client winning headers.
> + * debuginfod.cxx (handle_buildid_f_match): remove X-DEBUGINFOD-FILE
> + path. Add X-DEBUGINFOD-FILE and X-DEBUGINFOD-SIZE headers.
> + (handle_buildid_r_match): remove X-DEBUGINFOD-FILE path. Add
> + X-DEBUGINFOD-FILE, X-DEBUGINFOD-SIZE
> + headers, and X-ARCHIVE headers.
> +
> 2021-07-26 Noah Sanci
>
> PR27982
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index d41723ce..9c16e7a3 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -127,6 +127,7 @@ struct debuginfod_client
> timeout or other info gotten from environment variables, the
> handle data, etc. So those don't have to be reparsed and
> recreated on each request. */
> + char * winning_headers;
> };
>
> /* The cache_clean_interval_s file within the debuginfod cache specifies
> @@ -183,6 +184,8 @@ struct handle_data
> to the cache. Used to ensure that a file is not downloaded from
> multiple servers unnecessarily. */
>CURL **target_handle;
> + /* Response http headers for this client handle, sent from the server */
> + char *response_data;
> };
I think it will be convenient to also add a size_t response_data_size
field. See below.
> +static size_t
> +header_callback (char * buffer, size_t size, size_t numitems, void *
> userdata)
> +{
> + if (size != 1)
> +return 0;
> + /* Temporary buffer for realloc */
> + char *temp = NULL;
> + size_t userlen = 0;
> + if (*(char**)userdata == NULL)
> +{
> + temp = malloc(numitems+1);
> + if (temp == NULL)
> +return 0;
> + memset(temp, '\0', numitems+1);
> +}
> + else
> +{
> + userlen = strlen(*(char**)userdata);
> + temp = realloc(*(char**)userdata, userlen + numitems + 1);
> + if (temp == NULL)
> + return 0;
> +}
> + strncat(temp, buffer, numitems);
> + *(char**)userdata = temp;
> + return numitems;
> +}
I found https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html
Maybe you could add that as comment for future readers.
In the above userdata points to data[i].response_data. Which means the
length/size needs to recalculated each time. Also the data is added
with strncat which needs to walk the whole buffer again.
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).
> /* Query each of the server URLs found in $DEBUGINFOD_URLS for the file
> with the specified build-id, type (debuginfo, executable or source)
> and filename. filename may be NULL. If found, return a file
> @@ -936,10 +966,13 @@ debuginfod_query_server (debuginfod_client *c,
> curl_easy_setopt (data[i].handle, CURLOPT_LOW_SPEED_LIMIT,
> 100 * 1024L);
> }
> + data[i].response_data = NULL;
>curl_easy_setopt(data[i].handle, CURLOPT_FILETIME, (long) 1);
>curl_easy_setopt(data[i].handle, CURLOPT_FOLLOWLOCATION, (long) 1);
>curl_easy_setopt(data[i].handle, CURLOPT_FAILONERROR, (long) 1);
>curl_easy_setopt(data[i].handle, CURLOPT_NOSIGNAL, (long) 1);
> + curl_easy_setopt(data[i].handle, C
Re: [PATCH] libdw: set address size, offset size and version on fake CUs
Hi, On Wed, Sep 08, 2021 at 10:14:02PM +0200, Mark Wielaard wrote: > There are three "fake CUs" that are associated with .debug_loc, > .debug_loclist and .debug_addr. These fake CUs are used for "fake > attributes" to provide values that are stored in these sections > instead of in the .debug_info section. These fake CUs didn't have the > address size, offset size and DWARF version set. This meant that > values that depended on those properties might not be interpreted > correctly. One example was the value associated with a DW_OP_addrx > (which comes from the .debug_addr section). > > Add a testcase using varlocs to test that addresses can correctly be > retrieved for gcc/clang, DWARF4/5 and 32/64 bits objects. > > https://sourceware.org/bugzilla/show_bug.cgi?id=28220 Added ChangeLog entries and pushed. Cheers, Mark
[Bug libdw/28220] dwarf_location_attr returns high-bit junk from .debug_addr when fetching 32-bit addresses
https://sourceware.org/bugzilla/show_bug.cgi?id=28220 Mark Wielaard changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #6 from Mark Wielaard --- commit 52b0d9caf5575a62322c9fbe920b69444dd09162 Author: Mark Wielaard Date: Thu Aug 26 19:05:45 2021 +0200 libdw: set address size, offset size and version on fake CUs There are three "fake CUs" that are associated with .debug_loc, .debug_loclist and .debug_addr. These fake CUs are used for "fake attributes" to provide values that are stored in these sections instead of in the .debug_info section. These fake CUs didn't have the address size, offset size and DWARF version set. This meant that values that depended on those properties might not be interpreted correctly. One example was the value associated with a DW_OP_addrx (which comes from the .debug_addr section). Add a testcase using varlocs to test that addresses can correctly be retrieved for gcc/clang, DWARF4/5 and 32/64 bits objects. https://sourceware.org/bugzilla/show_bug.cgi?id=28220 Signed-off-by: Mark Wielaard -- You are receiving this mail because: You are on the CC list for the bug.
Re: [PATCH] lib: Make error.c more like error(3)
Hi Colin,
On Fri, Sep 10, 2021 at 11:07:16AM -0700, Colin Cross via Elfutils-devel wrote:
> Fix some issues with the error reimplementation to make it match
> the specification for error(3).
>
> Flush stdout before printing to stderr. Also flush stderr afterwards,
> which is not specified in the man page for error(3), but is what
> bionic does.
>
> error(3) prints strerror(errnum) if and only if errnum is nonzero,
> but verr prints strerror(errno) unconditionaly. When errnum is nonzero
> copy it to errno and use verr, and when it is not set use verrx that
> doesn't print errno.
>
> error(3) only exits if status is nonzero, but verr exits uncondtionally.
> Use vwarn/vwarnx when status is zero, which don't exit.
>
> Signed-off-by: Colin Cross
This is really nice. Thanks. I pushed it with a ChangeLog entry and
reformatted the patch to GNU style as attached.
Note that there are still some small differences between "real" error
and our error implementation, which don't seem to matter in practice,
but do cause some testsuite failures because of silly
formatting. e.g. run-stack-i.sh fails with:
-stack: tid 13654: shown max number of frames (6, use -n 0 for unlimited)
+/home/mark/src/elfutils/src/stack: tid 13654: shown max number of frames (6,
use -n 0 for unlimited)
I have not tries to fix these. The testcases probably should be
slightly less picky about the exact error message format.
Cheers,
Mark
>From 7582a0d3e09ee154961bbba9285a224e5d09f407 Mon Sep 17 00:00:00 2001
From: Colin Cross
Date: Fri, 10 Sep 2021 11:07:16 -0700
Subject: [PATCH] lib: Make error.c more like error(3)
Fix some issues with the error reimplementation to make it match
the specification for error(3).
Flush stdout before printing to stderr. Also flush stderr afterwards,
which is not specified in the man page for error(3), but is what
bionic does.
error(3) prints strerror(errnum) if and only if errnum is nonzero,
but verr prints strerror(errno) unconditionaly. When errnum is nonzero
copy it to errno and use verr, and when it is not set use verrx that
doesn't print errno.
error(3) only exits if status is nonzero, but verr exits uncondtionally.
Use vwarn/vwarnx when status is zero, which don't exit.
Signed-off-by: Colin Cross
---
lib/ChangeLog | 5 +
lib/error.c | 32 +---
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/lib/ChangeLog b/lib/ChangeLog
index 96eaa330..c72452b1 100644
--- a/lib/ChangeLog
+++ b/lib/ChangeLog
@@ -1,3 +1,8 @@
+2021-09-10 Colin Cross
+
+ * error.c (error): Call fflush on stdout and stderr. Setup errno and
+ call verr, verrx, vwarn or vwarnx based on status and errnum.
+
2021-09-06 Dmitry V. Levin
* color.c (parse_opt): Replace asprintf followed by error(EXIT_FAILURE)
diff --git a/lib/error.c b/lib/error.c
index 75e964fd..5186fc15 100644
--- a/lib/error.c
+++ b/lib/error.c
@@ -29,7 +29,9 @@
#include
#if !defined(HAVE_ERROR_H) && defined(HAVE_ERR_H)
+#include
#include
+#include
#include
#include
@@ -37,13 +39,37 @@ unsigned int error_message_count = 0;
void error(int status, int errnum, const char *format, ...) {
va_list argp;
+ int saved_errno = errno;
+
+ fflush (stdout);
va_start(argp, format);
- verr(status, format, argp);
+ if (status)
+{
+ if (errnum)
+{
+ errno = errnum;
+ verr (status, format, argp);
+}
+ else
+verrx (status, format, argp);
+}
+ else
+{
+ if (errnum)
+{
+ errno = errnum;
+ vwarn (format, argp);
+}
+ else
+vwarnx (format, argp);
+}
va_end(argp);
- if (status)
-exit(status);
+ fflush (stderr);
+
++error_message_count;
+
+ errno = saved_errno;
}
#endif
--
2.32.0
Buildbot failure in Wildebeest Builder on whole buildset
The Buildbot has detected a new failure on builder elfutils-centos-x86_64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/1/builds/835 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: centos-x86_64 Build Reason: Blamelist: Colin Cross BUILD FAILED: failed test (failure) Sincerely, -The BuildbotThe Buildbot has detected a new failure on builder elfutils-fedora-ppc64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/12/builds/777 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: fedora-ppc64 Build Reason: Blamelist: Colin Cross BUILD FAILED: failed test (failure) Sincerely, -The Buildbot
Re: Buildbot failure in Wildebeest Builder on whole buildset
Hi,
On Sun, Sep 12, 2021 at 09:56:39PM +, build...@builder.wildebeest.org wrote:
> The Buildbot has detected a new failure on builder elfutils-centos-x86_64
> while building elfutils.
> Full details are available at:
> https://builder.wildebeest.org/buildbot/#builders/1/builds/835
>
> Buildbot URL: https://builder.wildebeest.org/buildbot/
>
> Worker for this Build: centos-x86_64
>
> Build Reason:
> Blamelist: Colin Cross
>
> BUILD FAILED: failed test (failure)
> [...]
> Full details are available at:
> https://builder.wildebeest.org/buildbot/#builders/12/builds/777
This is certainly not caused by Colin's patch. It is the same failure
on two different systems, in related, but different tests.
run-debuginfod-federation-link.sh
run-debuginfod-federation-metrics.sh
run-debuginfod-federation-sqlite.sh
All have the following subtest:
# confirm that first server can't resolve symlinked info in L/ but second can
BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
-a L/foo | grep 'Build ID' | cut -d ' ' -f 7`
file L/foo
file -L L/foo
export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
rm -rf $DEBUGINFOD_CACHE_PATH
testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID &&
false || true
rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit
file
export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
Where L/foo is created with:
ln -s ${abs_builddir}/dwfllines L/foo # any program not used elsewhere in
this test
Both debuginfod servers have L in their scan path, but only the one on
PORT2 has -L L. So the one on PORT1 should not resolve the foo
symlink, but the one on PORT2 doesn't.
For some reason it seems even the server on PORT2 doesn't record L/foo
sometimes (but not always).
I haven't figured out why. Both failures above have extended logs, so
maybe someone else can figure out what is going wrong in the failure
case?
https://builder.wildebeest.org/buildbot/#/builders/12/builds/777/steps/8/logs/test-suite_log
https://builder.wildebeest.org/buildbot/#/builders/1/builds/835/steps/8/logs/test-suite_log
Cheers,
Mark
[COMMITTED] debuginfod: Add endl after "fdcache emergency flush for filling tmpdir" log
Without the endl the next log message will not start on its own line.
Signed-off-by: Mark Wielaard
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod.cxx | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 7e221f54..1173f9cd 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2021-09-12 Mark Wielaard
+
+ * debuginfod.cxx (libarchive_fdcache::lookup): Add endl after
+ obatched(clog) line.
+
2021-09-06 Dmitry V. Levin
* debuginfod-client.c (debuginfod_begin): Remove cast of calloc return
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 3269f657..6cc9f777 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1347,7 +1347,7 @@ public:
if (statfs_free_enough_p(tmpdir, "tmpdir", fdcache_mintmp))
{
inc_metric("fdcache_op_count","op","emerg-flush");
-obatched(clog) << "fdcache emergency flush for filling tmpdir";
+obatched(clog) << "fdcache emergency flush for filling tmpdir" << endl;
this->limit(0, 0, 0, 0); // emergency flush
}
else if (fd >= 0)
--
2.32.0
Buildbot failure in Wildebeest Builder on whole buildset
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 Build Reason: Blamelist: Mark Wielaard BUILD FAILED: failed test (failure) Sincerely, -The Buildbot
