[Bug debuginfod/28034] %-escape url characters

[Bug debuginfod/28034] %-escape url characters

[Noah Sanci]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

Hello,

Please find the patch for bug 28034 attached.

Noah

[-- Attachment #2: 0001-debuginfod-PR28034-escape-url-characters.patch --]
[-- Type: text/x-patch, Size: 8097 bytes --]

From 1e2340a9732bdc8f9a7a207a870e6815c770c23c Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 16 Jul 2021 15:16:20 -0400
Subject: [PATCH] debuginfod: PR28034 - %-escape url characters

When requesting some source files, some URL-inconvenient chars
sometimes pop up.  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.  We need to suppress this HTTP URL
processing step.  Also worth checking that %HEX decoding is also
suppressed.

https://sourceware.org/bugzilla/show_bug.cgi?id=28034

Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
 debuginfod/ChangeLog           |  6 ++++++
 debuginfod/debuginfod-client.c | 12 ++++++++---
 doc/debuginfod.8               |  1 +
 tests/ChangeLog                |  6 ++++++
 tests/run-debuginfod-find.sh   | 37 ++++++++++++++++++----------------
 5 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index e71436ca..aad35a4e 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	PR28034
+	* debuginfod-client.c (debuginfod_query_server): % escape filename
+	so the completed url is processed properly.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f12f609c..e30f73eb 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -831,9 +831,15 @@ debuginfod_query_server (debuginfod_client *c,
       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)/* 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);
+          snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
+                   slashbuildid, 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);
diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
index 1adf703a..90cdcf94 100644
--- a/doc/debuginfod.8
+++ b/doc/debuginfod.8
@@ -299,6 +299,7 @@ l l.
 \../bar/foo.c AT_comp_dir=/zoo/	/buildid/BUILDID/source/zoo//../bar/foo.c
 .TE
 
+Note: the client should %-escape characters in /SOURCE/FILE that are not shown as "unreserved" in section 2.3 of RFC3986.
 .SS /metrics
 
 This endpoint returns a Prometheus formatted text/plain dump of a
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 1460b422..cfa0dee4 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	PR28034
+	* run-debuginfod-find.sh: Added a test case ensuring files with %
+	escapable characters in their paths are accessible.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 73bbe65b..11c1a1a1 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -44,6 +44,7 @@ cleanup()
   if [ $PID2 -ne 0 ]; then kill $PID2; wait $PID2; fi
   if [ $PID3 -ne 0 ]; then kill $PID3; wait $PID3; fi
   if [ $PID4 -ne 0 ]; then kill $PID4; wait $PID4; fi
+  sleep 5; # Give time to ensure debuginfod cleans and closes sqlite databases.
   rm -rf F R D L Z ${PWD}/foobar ${PWD}/mocktree ${PWD}/.client_cache* ${PWD}/tmp*
   exit_cleanup
 }
@@ -54,7 +55,7 @@ trap cleanup 0 1 2 3 5 9 15
 errfiles_list=
 err() {
     echo ERROR REPORTS
-    for ports in $PORT1 $PORT2
+    for ports in $PORT1 $PORT2 $PORT3
     do
         echo ERROR REPORT $port metrics
         curl -s http://127.0.0.1:$port/metrics
@@ -149,18 +150,18 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
 # Compile a simple program, strip its debuginfo and save the build-id.
 # Also move the debuginfo into another directory so that elfutils
 # cannot find it without debuginfod.
-echo "int main() { return 0; }" > ${PWD}/prog.c
-tempfiles prog.c
+echo "int main() { return 0; }" > ${PWD}/p+r%o\$g.c
+tempfiles p+r%o\$g.c
 # Create a subdirectory to confound source path names
 mkdir foobar
-gcc -Wl,--build-id -g -o prog ${PWD}/foobar///./../prog.c
-testrun ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
+gcc -Wl,--build-id -g -o p+r%o\$g ${PWD}/foobar///./../p+r%o\$g.c
+testrun ${abs_top_builddir}/src/strip -g -f p+r%o\$g.debug ${PWD}/p+r%o\$g
 BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
-          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
+          -a p+r%o\\$g | grep 'Build ID' | cut -d ' ' -f 7`
 
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
-mv prog F
-mv prog.debug F
+mv p+r%o\$g F
+mv p+r%o\$g.debug F
 kill -USR1 $PID1
 # Wait till both files are in the index.
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
@@ -171,7 +172,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
 
 # Test whether elfutils, via the debuginfod client library dlopen hooks,
 # is able to fetch debuginfo from the local debuginfod.
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 ########################################################################
 
@@ -213,22 +214,22 @@ fi
 # Test whether debuginfod-find is able to fetch those files.
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
-cmp $filename F/prog.debug
+cmp $filename F/p+r%o\$g.debug
 if [ -w $filename ]; then
     echo "cache file writable, boo"
     err
 fi
 
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
-cmp $filename F/prog
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/p+r%o\\$g`
+cmp $filename F/p+r%o\$g
 
 # raw source filename
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 # and also the canonicalized one
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 
 ########################################################################
@@ -672,7 +673,7 @@ touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee
 echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
 echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
 
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true
 
@@ -725,6 +726,7 @@ PID4=$!
 wait_ready $PORT3 'ready' 1
 tempfiles vlog$PORT3
 errfiles vlog$PORT3
+tempfiles $DB.backup*
 
 kill -USR2 $PID4
 wait_ready $PORT3 'thread_work_total{role="groom"}' 1
@@ -738,6 +740,7 @@ wait_ready $PORT3 'groom{statistic="files scanned (#)"}' 0
 wait_ready $PORT3 'groom{statistic="files scanned (mb)"}' 0
 
 kill $PID4
+PID4=0
 
 ########################################################################
 # set up tests for retrying failed queries.
-- 
2.31.1

Re: [Bug debuginfod/28034] %-escape url characters

[Mark Wielaard]

Hi Noah,

On Mon, Jul 19, 2021 at 10:53:17AM -0400, Noah Sanci via Elfutils-devel wrote:
> When requesting some source files, some URL-inconvenient chars
> sometimes pop up.  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.  We need to suppress this HTTP URL
> processing step.  Also worth checking that %HEX decoding is also
> suppressed.

I find this description slightly confusing. It ends with "Also worth
checking..." but that is actually what is done in this patch. The part
before that about what debuginfod/microhttpd does on the server side
is interesting, but not really what this patch is about (just why this
patch is necessary, but it seems necessary on the client side always
whatever server is used).

Could you rewrite the commit message to describe what is done in this
patch?

> https://sourceware.org/bugzilla/show_bug.cgi?id=28034

> +2021-07-16  Noah Sanci  <nsanci@redhat.com>
> +
> +	PR28034
> +	* debuginfod-client.c (debuginfod_query_server): % escape filename
> +	so the completed url is processed properly.

> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index f12f609c..e30f73eb 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -831,9 +831,15 @@ debuginfod_query_server (debuginfod_client *c,
>        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)/* 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);

curl_easy_escape () can return NULL on failure.

> +          snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> +                   slashbuildid, build_id_bytes, type, escaped_string);
> +          curl_free(escaped_string);
> +        }

I note that filename is actually the full path component of the URL so
includes slashes ('/'). curl_easy_escape seems to convert these to %2F
(if I am correct). Is this intended?

>        else
>          snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
>                   slashbuildid, build_id_bytes, type);
> diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
> index 1adf703a..90cdcf94 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -299,6 +299,7 @@ l l.
>  \../bar/foo.c AT_comp_dir=/zoo/	/buildid/BUILDID/source/zoo//../bar/foo.c
>  .TE
>  
> +Note: the client should %-escape characters in /SOURCE/FILE that are not shown as "unreserved" in section 2.3 of RFC3986.

This is a very long line. Could you break it up?
Also, maybe just give the information instead of only a reference.
(The "unreserved" characters are "a"-"z"", "A"-"Z", "0"-"9", "-", ".", "_" and "~")

Also same question as above. slash ('/') is not an unreserved
character, should it be encoded?

>  .SS /metrics
>  
>  This endpoint returns a Prometheus formatted text/plain dump of a
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 1460b422..cfa0dee4 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-07-16  Noah Sanci  <nsanci@redhat.com>
> +
> +	PR28034
> +	* run-debuginfod-find.sh: Added a test case ensuring files with %
> +	escapable characters in their paths are accessible.

There are also a couple of changes (fixes?) to the testcases.
Could those be split out?

>  2021-07-06  Alice Zhang  <alizhang@redhat.com>
>  
>          PR27531
> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> index 73bbe65b..11c1a1a1 100755
> --- a/tests/run-debuginfod-find.sh
> +++ b/tests/run-debuginfod-find.sh
> @@ -44,6 +44,7 @@ cleanup()
>    if [ $PID2 -ne 0 ]; then kill $PID2; wait $PID2; fi
>    if [ $PID3 -ne 0 ]; then kill $PID3; wait $PID3; fi
>    if [ $PID4 -ne 0 ]; then kill $PID4; wait $PID4; fi
> +  sleep 5; # Give time to ensure debuginfod cleans and closes sqlite databases.
>    rm -rf F R D L Z ${PWD}/foobar ${PWD}/mocktree ${PWD}/.client_cache* ${PWD}/tmp*
>    exit_cleanup
>  }

This seems a testsuite fixup?

> @@ -54,7 +55,7 @@ trap cleanup 0 1 2 3 5 9 15
>  errfiles_list=
>  err() {
>      echo ERROR REPORTS
> -    for ports in $PORT1 $PORT2
> +    for ports in $PORT1 $PORT2 $PORT3
>      do
>          echo ERROR REPORT $port metrics
>          curl -s http://127.0.0.1:$port/metrics

As is this?

> @@ -149,18 +150,18 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
>  # Compile a simple program, strip its debuginfo and save the build-id.
>  # Also move the debuginfo into another directory so that elfutils
>  # cannot find it without debuginfod.
> -echo "int main() { return 0; }" > ${PWD}/prog.c
> -tempfiles prog.c
> +echo "int main() { return 0; }" > ${PWD}/p+r%o\$g.c
> +tempfiles p+r%o\$g.c
>  # Create a subdirectory to confound source path names
>  mkdir foobar
> -gcc -Wl,--build-id -g -o prog ${PWD}/foobar///./../prog.c
> -testrun ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
> +gcc -Wl,--build-id -g -o p+r%o\$g ${PWD}/foobar///./../p+r%o\$g.c
> +testrun ${abs_top_builddir}/src/strip -g -f p+r%o\$g.debug ${PWD}/p+r%o\$g
>  BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
> -          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
> +          -a p+r%o\\$g | grep 'Build ID' | cut -d ' ' -f 7`
>  
>  wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
> -mv prog F
> -mv prog.debug F
> +mv p+r%o\$g F
> +mv p+r%o\$g.debug F
>  kill -USR1 $PID1
>  # Wait till both files are in the index.
>  wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
> @@ -171,7 +172,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
>  
>  # Test whether elfutils, via the debuginfod client library dlopen hooks,
>  # is able to fetch debuginfo from the local debuginfod.
> -testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
>  
>  ########################################################################
>  
> @@ -213,22 +214,22 @@ fi
>  # Test whether debuginfod-find is able to fetch those files.
>  rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
>  filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
> -cmp $filename F/prog.debug
> +cmp $filename F/p+r%o\$g.debug
>  if [ -w $filename ]; then
>      echo "cache file writable, boo"
>      err
>  fi
>  
> -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
> -cmp $filename F/prog
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/p+r%o\\$g`
> +cmp $filename F/p+r%o\$g
>  
>  # raw source filename
> -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../prog.c`
> -cmp $filename  ${PWD}/prog.c
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../p+r%o\\$g.c`
> +cmp $filename  ${PWD}/p+r%o\$g.c
>  
>  # and also the canonicalized one
> -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
> -cmp $filename  ${PWD}/prog.c
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/p+r%o\\$g.c`
> +cmp $filename  ${PWD}/p+r%o\$g.c
>  
>  
>  ########################################################################
> @@ -672,7 +673,7 @@ touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee
>  echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
>  echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
>  
> -testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
>  
>  testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true
>  
> @@ -725,6 +726,7 @@ PID4=$!
>  wait_ready $PORT3 'ready' 1
>  tempfiles vlog$PORT3
>  errfiles vlog$PORT3
> +tempfiles $DB.backup*

???

>  kill -USR2 $PID4
>  wait_ready $PORT3 'thread_work_total{role="groom"}' 1
> @@ -738,6 +740,7 @@ wait_ready $PORT3 'groom{statistic="files scanned (#)"}' 0
>  wait_ready $PORT3 'groom{statistic="files scanned (mb)"}' 0
>  
>  kill $PID4
> +PID4=0

And this?
Might need to be

  wait $PID
  PID4=0

? And maybe double check the other kills in the test?

Cheers,

Mark

Re: [Bug debuginfod/28034] %-escape url characters

[Frank Ch. Eigler]

Hi -

> Could you rewrite the commit message to describe what is done in this
> patch?

(Yeah, Noah's commit text on his branch was corrected.)

> [...]
> I note that filename is actually the full path component of the URL so
> includes slashes ('/'). curl_easy_escape seems to convert these to %2F
> (if I am correct). Is this intended?

It's harmless.

> > +Note: the client should %-escape characters in /SOURCE/FILE that are not shown as "unreserved" in section 2.3 of RFC3986.
> 
> This is a very long line. Could you break it up?
> Also, maybe just give the information instead of only a reference.
> (The "unreserved" characters are "a"-"z"", "A"-"Z", "0"-"9", "-", ".", "_" and "~")
> Also same question as above. slash ('/') is not an unreserved
> character, should it be encoded?

As we know from the status quo working for a year+, it doesn't matter
for "/".  But RFC3986 does not give a character class that corresponds
exactly to what MUST be encoded, so for documentation purposes this
simple SHOULD guidance seems fine.


- FChE

Re: [Bug debuginfod/28034] %-escape url characters

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

Hi Noah,

On Tue, Jul 20, 2021 at 07:50:11PM +0200, Mark Wielaard wrote:
> > +	* run-debuginfod-find.sh: Added a test case ensuring files with %
> > +	escapable characters in their paths are accessible.
> 
> There are also a couple of changes (fixes?) to the testcases.
> Could those be split out?

I think you almost had the right fix for a race in killing the last
debuginfod server. Does the attached work for you?

Thanks,

Mark

[-- Attachment #2: 0001-tests-wait-for-PID4-before-setting-to-zero.patch --]
[-- Type: text/x-diff, Size: 1402 bytes --]

From 83b7eb24a5796a4aecc5d32eb0c3f459788c4690 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Tue, 20 Jul 2021 20:50:48 +0200
Subject: [PATCH] tests: wait for PID4 before setting to zero

A debuginfod server might take a while to shut down to clean and close
the sqlite databases. Wait for the process after killing it and
clearing the PID variable so it won't be killed again.

Reported-by: Noah Sanci <nsanci@redhat.com>
Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 tests/ChangeLog              | 4 ++++
 tests/run-debuginfod-find.sh | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 1196d6b2..b0303e00 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,7 @@
+2021-07-20  Mark Wielaard  <mark@klomp.org>
+
+	* tests/run-debuginfod-find.sh: wait for PID4 before setting to zero.
+
 2021-06-28  Noah Sanci <nsanci@redhat.com>
 
 	PR25978
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 1d664be9..23eac329 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -754,6 +754,8 @@ wait_ready $PORT3 'groom{statistic="files scanned (#)"}' 0
 wait_ready $PORT3 'groom{statistic="files scanned (mb)"}' 0
 
 kill $PID4
+wait $PID4
+PID4=0
 
 ########################################################################
 # set up tests for retrying failed queries.
-- 
2.32.0

Re: [Bug debuginfod/28034] %-escape url characters

[Noah Sanci]

[-- Attachment #1: Type: text/plain, Size: 5851 bytes --]

Hello,

> On Mon, Jul 19, 2021 at 10:53:17AM -0400, Noah Sanci via Elfutils-devel wrote:
> > When requesting some source files, some URL-inconvenient chars
> > sometimes pop up.  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.  We need to suppress this HTTP URL
> > processing step.  Also worth checking that %HEX decoding is also
> > suppressed.
>
> I find this description slightly confusing. It ends with "Also worth
> checking..." but that is actually what is done in this patch. The part
> before that about what debuginfod/microhttpd does on the server side
> is interesting, but not really what this patch is about (just why this
> patch is necessary, but it seems necessary on the client side always
> whatever server is used).

Thank you for bringing this to my attention. Fixed

> curl_easy_escape () can return NULL on failure.
>

Fixed.

> > +Note: the client should %-escape characters in /SOURCE/FILE that are not shown as "unreserved" in section 2.3 of RFC3986.

Please read the new note just to ensure that it's reasonable, as my
updated note specifies some characters that will be percent escaped.
Otherwise fixed.

> > @@ -149,18 +150,18 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
> >  # Compile a simple program, strip its debuginfo and save the build-id.
> >  # Also move the debuginfo into another directory so that elfutils
> >  # cannot find it without debuginfod.
> > -echo "int main() { return 0; }" > ${PWD}/prog.c
> > -tempfiles prog.c
> > +echo "int main() { return 0; }" > ${PWD}/p+r%o\$g.c
> > +tempfiles p+r%o\$g.c
> >  # Create a subdirectory to confound source path names
> >  mkdir foobar
> > -gcc -Wl,--build-id -g -o prog ${PWD}/foobar///./../prog.c
> > -testrun ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
> > +gcc -Wl,--build-id -g -o p+r%o\$g ${PWD}/foobar///./../p+r%o\$g.c
> > +testrun ${abs_top_builddir}/src/strip -g -f p+r%o\$g.debug ${PWD}/p+r%o\$g
> >  BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
> > -          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
> > +          -a p+r%o\\$g | grep 'Build ID' | cut -d ' ' -f 7`
> >
> >  wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
> > -mv prog F
> > -mv prog.debug F
> > +mv p+r%o\$g F
> > +mv p+r%o\$g.debug F
> >  kill -USR1 $PID1
> >  # Wait till both files are in the index.
> >  wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
> > @@ -171,7 +172,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
> >
> >  # Test whether elfutils, via the debuginfod client library dlopen hooks,
> >  # is able to fetch debuginfo from the local debuginfod.
> > -testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> > +testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
> >
> >  ########################################################################
> >
> > @@ -213,22 +214,22 @@ fi
> >  # Test whether debuginfod-find is able to fetch those files.
> >  rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> >  filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
> > -cmp $filename F/prog.debug
> > +cmp $filename F/p+r%o\$g.debug
> >  if [ -w $filename ]; then
> >      echo "cache file writable, boo"
> >      err
> >  fi
> >
> > -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
> > -cmp $filename F/prog
> > +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/p+r%o\\$g`
> > +cmp $filename F/p+r%o\$g
> >
> >  # raw source filename
> > -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../prog.c`
> > -cmp $filename  ${PWD}/prog.c
> > +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../p+r%o\\$g.c`
> > +cmp $filename  ${PWD}/p+r%o\$g.c
> >
> >  # and also the canonicalized one
> > -filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
> > -cmp $filename  ${PWD}/prog.c
> > +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/p+r%o\\$g.c`
> > +cmp $filename  ${PWD}/p+r%o\$g.c
> >
> >
> >  ########################################################################
> > @@ -672,7 +673,7 @@ touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee
> >  echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
> >  echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
> >
> > -testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> > +testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
> >
> >  testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true
> >
> > @@ -725,6 +726,7 @@ PID4=$!
> >  wait_ready $PORT3 'ready' 1
> >  tempfiles vlog$PORT3
> >  errfiles vlog$PORT3
> > +tempfiles $DB.backup*
>
> ???
>

Hopefully the updated changelog message clarifies these changes.

> >  kill -USR2 $PID4
> >  wait_ready $PORT3 'thread_work_total{role="groom"}' 1
> > @@ -738,6 +740,7 @@ wait_ready $PORT3 'groom{statistic="files scanned (#)"}' 0
> >  wait_ready $PORT3 'groom{statistic="files scanned (mb)"}' 0
> >
> >  kill $PID4
> > +PID4=0
>
> And this?
> Might need to be
>
>   wait $PID
>   PID4=0

The small test suite fixes have been removed from the script, however
I would like to update the test suite ASAP. Can I just commit a new
branch titled something like 'elfutils-test-fixes' without a specific
pr?

Noah

[-- Attachment #2: 0001-debuginfod-PR28034-client-side-escape-url-characters.patch --]
[-- Type: text/x-patch, Size: 7532 bytes --]

From 26d39eee2ecc030ac7e488977092f62afd72e903 Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 16 Jul 2021 15:16:20 -0400
Subject: [PATCH] debuginfod: PR28034 - client-side %-escape url characters

When requesting some source files, some URL-inconvenient chars
sometimes pop up.  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.
In order to ensure the server properly parses urls such as this one,
%-escape characters on the client side so that the correct url
is preserved and properly processed on the server side.

https://sourceware.org/bugzilla/show_bug.cgi?id=28034

Signed-off-by: Noah Sanci <nsanci@redhat.com>

a
---
 debuginfod/ChangeLog           |  6 ++++++
 debuginfod/debuginfod-client.c | 17 ++++++++++++++---
 doc/debuginfod.8               |  4 ++++
 tests/ChangeLog                |  9 +++++++++
 tests/run-debuginfod-find.sh   | 32 ++++++++++++++++----------------
 5 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index e71436ca..aad35a4e 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	PR28034
+	* debuginfod-client.c (debuginfod_query_server): % escape filename
+	so the completed url is processed properly.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f12f609c..64936acd 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -831,9 +831,20 @@ debuginfod_query_server (debuginfod_client *c,
       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)/* 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 out1;
+            }
+          snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
+                   slashbuildid, 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);
diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
index 1adf703a..ee8f76ae 100644
--- a/doc/debuginfod.8
+++ b/doc/debuginfod.8
@@ -299,6 +299,10 @@ l l.
 \../bar/foo.c AT_comp_dir=/zoo/	/buildid/BUILDID/source/zoo//../bar/foo.c
 .TE
 
+Note: the client should %-escape characters in /SOURCE/FILE that are
+not shown as "unreserved" in section 2.3 of RFC3986. Some characters
+that will be escaped include "+", "\\", "$", "!", the 'space' character,
+and ";". RFC3986 includes a more comprehensive list of these characters.
 .SS /metrics
 
 This endpoint returns a Prometheus formatted text/plain dump of a
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 1460b422..0840e7af 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,12 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	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.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 73bbe65b..ecf3195e 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -149,18 +149,18 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
 # Compile a simple program, strip its debuginfo and save the build-id.
 # Also move the debuginfo into another directory so that elfutils
 # cannot find it without debuginfod.
-echo "int main() { return 0; }" > ${PWD}/prog.c
-tempfiles prog.c
+echo "int main() { return 0; }" > ${PWD}/p+r%o\$g.c
+tempfiles p+r%o\$g.c
 # Create a subdirectory to confound source path names
 mkdir foobar
-gcc -Wl,--build-id -g -o prog ${PWD}/foobar///./../prog.c
-testrun ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
+gcc -Wl,--build-id -g -o p+r%o\$g ${PWD}/foobar///./../p+r%o\$g.c
+testrun ${abs_top_builddir}/src/strip -g -f p+r%o\$g.debug ${PWD}/p+r%o\$g
 BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
-          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
+          -a p+r%o\\$g | grep 'Build ID' | cut -d ' ' -f 7`
 
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
-mv prog F
-mv prog.debug F
+mv p+r%o\$g F
+mv p+r%o\$g.debug F
 kill -USR1 $PID1
 # Wait till both files are in the index.
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
@@ -171,7 +171,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
 
 # Test whether elfutils, via the debuginfod client library dlopen hooks,
 # is able to fetch debuginfo from the local debuginfod.
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 ########################################################################
 
@@ -213,22 +213,22 @@ fi
 # Test whether debuginfod-find is able to fetch those files.
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
-cmp $filename F/prog.debug
+cmp $filename F/p+r%o\$g.debug
 if [ -w $filename ]; then
     echo "cache file writable, boo"
     err
 fi
 
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
-cmp $filename F/prog
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/p+r%o\\$g`
+cmp $filename F/p+r%o\$g
 
 # raw source filename
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 # and also the canonicalized one
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 
 ########################################################################
@@ -672,7 +672,7 @@ touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee
 echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
 echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
 
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true
 
-- 
2.31.1

Re: [Bug debuginfod/28034] %-escape url characters

[Noah Sanci]

[-- Attachment #1: Type: text/plain, Size: 41 bytes --]

Hello,

Here is a quick error fix.

Noah

[-- Attachment #2: 0001-debuginfod-PR28034-client-side-escape-url-characters.patch --]
[-- Type: text/x-patch, Size: 7472 bytes --]

From 26aaefe02f5d4ad9187a36d79304edb1b0b450df Mon Sep 17 00:00:00 2001
From: Noah Sanci <nsanci@redhat.com>
Date: Fri, 16 Jul 2021 15:16:20 -0400
Subject: [PATCH] debuginfod: PR28034 - client-side %-escape url characters

When requesting some source files, some URL-inconvenient chars
sometimes pop up.  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.
In order to ensure the server properly parses urls such as this one,
%-escape characters on the client side so that the correct url
is preserved and properly processed on the server side.

https://sourceware.org/bugzilla/show_bug.cgi?id=28034

Signed-off-by: Noah Sanci <nsanci@redhat.com>
---
 debuginfod/ChangeLog           |  6 ++++++
 debuginfod/debuginfod-client.c | 15 +++++++++++++--
 doc/debuginfod.8               |  4 ++++
 tests/ChangeLog                |  9 +++++++++
 tests/run-debuginfod-find.sh   | 32 ++++++++++++++++----------------
 5 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index e71436ca..aad35a4e 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	PR28034
+	* debuginfod-client.c (debuginfod_query_server): % escape filename
+	so the completed url is processed properly.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f12f609c..26ba1891 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -832,8 +832,19 @@ debuginfod_query_server (debuginfod_client *c,
         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);
+        {
+          /* 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 out1;
+            }
+          snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
+                   slashbuildid, 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);
diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
index 1adf703a..ee8f76ae 100644
--- a/doc/debuginfod.8
+++ b/doc/debuginfod.8
@@ -299,6 +299,10 @@ l l.
 \../bar/foo.c AT_comp_dir=/zoo/	/buildid/BUILDID/source/zoo//../bar/foo.c
 .TE
 
+Note: the client should %-escape characters in /SOURCE/FILE that are
+not shown as "unreserved" in section 2.3 of RFC3986. Some characters
+that will be escaped include "+", "\\", "$", "!", the 'space' character,
+and ";". RFC3986 includes a more comprehensive list of these characters.
 .SS /metrics
 
 This endpoint returns a Prometheus formatted text/plain dump of a
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 1460b422..0840e7af 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,12 @@
+2021-07-16  Noah Sanci  <nsanci@redhat.com>
+
+	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.
+
 2021-07-06  Alice Zhang  <alizhang@redhat.com>
 
         PR27531
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 73bbe65b..ecf3195e 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -149,18 +149,18 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
 # Compile a simple program, strip its debuginfo and save the build-id.
 # Also move the debuginfo into another directory so that elfutils
 # cannot find it without debuginfod.
-echo "int main() { return 0; }" > ${PWD}/prog.c
-tempfiles prog.c
+echo "int main() { return 0; }" > ${PWD}/p+r%o\$g.c
+tempfiles p+r%o\$g.c
 # Create a subdirectory to confound source path names
 mkdir foobar
-gcc -Wl,--build-id -g -o prog ${PWD}/foobar///./../prog.c
-testrun ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
+gcc -Wl,--build-id -g -o p+r%o\$g ${PWD}/foobar///./../p+r%o\$g.c
+testrun ${abs_top_builddir}/src/strip -g -f p+r%o\$g.debug ${PWD}/p+r%o\$g
 BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
-          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
+          -a p+r%o\\$g | grep 'Build ID' | cut -d ' ' -f 7`
 
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
-mv prog F
-mv prog.debug F
+mv p+r%o\$g F
+mv p+r%o\$g.debug F
 kill -USR1 $PID1
 # Wait till both files are in the index.
 wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
@@ -171,7 +171,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
 
 # Test whether elfutils, via the debuginfod client library dlopen hooks,
 # is able to fetch debuginfo from the local debuginfod.
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 ########################################################################
 
@@ -213,22 +213,22 @@ fi
 # Test whether debuginfod-find is able to fetch those files.
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
-cmp $filename F/prog.debug
+cmp $filename F/p+r%o\$g.debug
 if [ -w $filename ]; then
     echo "cache file writable, boo"
     err
 fi
 
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
-cmp $filename F/prog
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/p+r%o\\$g`
+cmp $filename F/p+r%o\$g
 
 # raw source filename
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/foobar///./../p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 # and also the canonicalized one
-filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
-cmp $filename  ${PWD}/prog.c
+filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/p+r%o\\$g.c`
+cmp $filename  ${PWD}/p+r%o\$g.c
 
 
 ########################################################################
@@ -672,7 +672,7 @@ touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee
 echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
 echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
 
-testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
+testrun ${abs_builddir}/debuginfod_build_id_find -e F/p+r%o\$g 1
 
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true
 
-- 
2.31.1

Re: [Bug debuginfod/28034] %-escape url characters

[Mark Wielaard]

Hi Noah,

On Wed, 2021-07-21 at 14:18 -0400, Noah Sanci via Elfutils-devel wrote:
> 
> > > +Note: the client should %-escape characters in /SOURCE/FILE that
> > > are not shown as "unreserved" in section 2.3 of RFC3986.
> 
> Please read the new note just to ensure that it's reasonable, as my
> updated note specifies some characters that will be percent escaped.
> Otherwise fixed.

Yes, that looks good.
As do the alloc error checks and the test cleanups.
I'll push the minor fixed version you posted.

Thanks,

Mark

Re: [Bug debuginfod/28034] %-escape url characters

[Mark Wielaard]

Hi Noah,

On Wed, 2021-07-21 at 15:44 -0400, Noah Sanci via Elfutils-devel wrote:
> Here is a quick error fix.

Thanks, looks good. Failure results should indeed be negative.
For the record the actual fix was:

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 64936acd..26ba1891 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -831,14 +831,14 @@ debuginfod_query_server (debuginfod_client *c,
       else
         slashbuildid = "/buildid";
 
-      if (filename)/* must start with / */
+      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;
+              rc = -ENOMEM;
               goto out1;
             }
           snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,

Pushed this version.

Thanks,

Mark