[PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Aaron Merey]

Currently debuginfod is disabled when the string of server URLs
is unset or set to be the empty string (via the $DEBUGINFOD_URLS
environment variable or the 'set debuginfod urls' gdb command).

Extend this functionality so that a whitespace-only URL also disables
debuginfod.

Modify a testcase to verify that a whitespace-only URL disables
debuginfod.
---
 gdb/debuginfod-support.c                        |  6 +++---
 .../gdb.debuginfod/fetch_src_and_symbols.exp    | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 4ce2e786b6a..f47ed515e28 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -173,10 +173,10 @@ get_debuginfod_client ()
 static bool
 debuginfod_is_enabled ()
 {
-  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  std::string urls = getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "";
 
-  if (urls == nullptr || urls[0] == '\0'
-      || debuginfod_enabled == debuginfod_off)
+  if (debuginfod_enabled == debuginfod_off
+      || urls.find_first_not_of (' ') == std::string::npos)
     return false;
 
   if (debuginfod_enabled == debuginfod_ask)
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index bd90bcd0cfe..8b1618c242b 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -307,9 +307,10 @@ proc local_url { } {
     # Empty URLS disables Debuginfod.
     setenv DEBUGINFOD_URLS ""
     clean_restart
+    set file_cmd "with confirm off -- file $binfile"
     # Disable confirmation to avoid having to deal with a query.  See
     # test_urls.
-    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
+    gdb_test_multiple $file_cmd "notice empty URL" {
 	-re -wrap "This GDB supports auto-downloading.*" {
 	    fail $gdb_test_name
 	}
@@ -318,9 +319,17 @@ proc local_url { } {
 	}
     }
 
-    test_urls "   " \
-        "" \
-        "notice whitespace URL"
+    # Whitespace-only URLS disables Debuginfod.
+    setenv DEBUGINFOD_URLS "    "
+    clean_restart
+    gdb_test_multiple $file_cmd "notice whitespace URL" {
+	-re -wrap "This GDB supports auto-downloading.*" {
+	    fail $gdb_test_name
+	}
+	-re -wrap "" {
+	    pass $gdb_test_name
+	}
+    }
 
     set url "http://127.0.0.1:$port"
 
-- 
2.35.1

Re: [PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> -  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
Aaron> +  std::string urls = getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "";

The ?: syntax is a GNU extension, not a standard feature.
 
Aaron> -  if (urls == nullptr || urls[0] == '\0'
Aaron> -      || debuginfod_enabled == debuginfod_off)
Aaron> +  if (debuginfod_enabled == debuginfod_off
Aaron> +      || urls.find_first_not_of (' ') == std::string::npos)
Aaron>      return false;

You could also just leave it as a const char * and use skip_spaces.
That would also skip tabs, which is probably desirable in some sense anyway.

Tom

Re: [PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Pedro Alves]

On 2022-04-27 22:46, Aaron Merey wrote:
>      clean_restart
> +    set file_cmd "with confirm off -- file $binfile"
>      # Disable confirmation to avoid having to deal with a query.  See
>      # test_urls.

Please make it such that the comment ends up before the new variable, as the comment is talking
about the "with confirm off" part.

> -    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
> +    gdb_test_multiple $file_cmd "notice empty URL" {

[PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Aaron Merey]

On Thu, Apr 28, 2022 at 11:34 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> -  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> Aaron> +  std::string urls = getenv (DEBUGINFOD_URLS_ENV_VAR) ?: "";
>
> The ?: syntax is a GNU extension, not a standard feature.
>
> Aaron> -  if (urls == nullptr || urls[0] == '\0'
> Aaron> -      || debuginfod_enabled == debuginfod_off)
> Aaron> +  if (debuginfod_enabled == debuginfod_off
> Aaron> +      || urls.find_first_not_of (' ') == std::string::npos)
> Aaron>      return false;
>
> You could also just leave it as a const char * and use skip_spaces.
> That would also skip tabs, which is probably desirable in some sense anyway.

Fixed.

On Thu, Apr 28, 2022 at 12:29 PM Pedro Alves <pedro@palves.net> wrote:
>
> On 2022-04-27 22:46, Aaron Merey wrote:
> >      clean_restart
> > +    set file_cmd "with confirm off -- file $binfile"
> >      # Disable confirmation to avoid having to deal with a query.  See
> >      # test_urls.
>
> Please make it such that the comment ends up before the new variable, as the comment is talking
> about the "with confirm off" part.

Fixed.

Aaron

From 0733ef7fdd4259da6814b7ee8404d14522c79d9e Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Wed, 27 Apr 2022 16:41:24 -0400

Currently debuginfod is disabled when the string of server URLs
is unset or set to be the empty string (via the $DEBUGINFOD_URLS
environment variable or the 'set debuginfod urls' gdb command).

Extend this functionality so that a whitespace-only URL also disables
debuginfod.

Modify a testcase to verify that a whitespace-only URL disables
debuginfod.
---
 gdb/debuginfod-support.c                        |  6 ++++--
 .../gdb.debuginfod/fetch_src_and_symbols.exp    | 17 +++++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 4ce2e786b6a..ec0d09c78fb 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -174,9 +174,11 @@ static bool
 debuginfod_is_enabled ()
 {
   const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  const char *url_start = skip_spaces (urls);
 
-  if (urls == nullptr || urls[0] == '\0'
-      || debuginfod_enabled == debuginfod_off)
+  if (debuginfod_enabled == debuginfod_off
+      || url_start == nullptr
+      || *url_start == '\0')
     return false;
 
   if (debuginfod_enabled == debuginfod_ask)
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index bd90bcd0cfe..74d026464b9 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -309,7 +309,8 @@ proc local_url { } {
     clean_restart
     # Disable confirmation to avoid having to deal with a query.  See
     # test_urls.
-    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
+    set file_cmd "with confirm off -- file $binfile"
+    gdb_test_multiple $file_cmd "notice empty URL" {
 	-re -wrap "This GDB supports auto-downloading.*" {
 	    fail $gdb_test_name
 	}
@@ -318,9 +319,17 @@ proc local_url { } {
 	}
     }
 
-    test_urls "   " \
-        "" \
-        "notice whitespace URL"
+    # Whitespace-only URLS disables Debuginfod.
+    setenv DEBUGINFOD_URLS "    "
+    clean_restart
+    gdb_test_multiple $file_cmd "notice whitespace URL" {
+	-re -wrap "This GDB supports auto-downloading.*" {
+	    fail $gdb_test_name
+	}
+	-re -wrap "" {
+	    pass $gdb_test_name
+	}
+    }
 
     set url "http://127.0.0.1:$port"
 
-- 
2.35.1

Re: [PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Pedro Alves]

On 2022-04-28 23:37, Aaron Merey wrote:

> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 4ce2e786b6a..ec0d09c78fb 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -174,9 +174,11 @@ static bool
>  debuginfod_is_enabled ()
>  {
>    const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> +  const char *url_start = skip_spaces (urls);

Does the rest of the function need to see "urls" with the leading space?  It seems like the
code further below will also skip leading spaces anyhow.  IOW, we can't make this be:

    urls = skip_spaces (urls);

or both lines replaced with:

    const char *urls = skip_spaces (getenv (DEBUGINFOD_URLS_ENV_VAR));

?

>  
> -  if (urls == nullptr || urls[0] == '\0'
> -      || debuginfod_enabled == debuginfod_off)
> +  if (debuginfod_enabled == debuginfod_off
> +      || url_start == nullptr
> +      || *url_start == '\0')
>      return false;
>  

[PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Aaron Merey]

On Fri, Apr 29, 2022 at 10:22 AM Pedro Alves <pedro@palves.net> wrote:
> Does the rest of the function need to see "urls" with the leading space?  It seems like the 
> code further below will also skip leading spaces anyhow.  IOW, we can't make this be: 
>
>     urls = skip_spaces (urls);
>
> or both lines replaced with:
>
>     const char *urls = skip_spaces (getenv (DEBUGINFOD_URLS_ENV_VAR));
>
> ?

I kept the "urls" with the leading whitespace so that we parse each URL 
consistent with debuginfod's use of ' ' as the sole URL delimiter 
(skip_space also skips tabs, newlines, etc).  However debuginfod severs
should not have URLs containing unescaped whitespace so it's really a 
moot point.

I've fixed this by combining skip_spaces and getenv on one line.

Aaron

From 4cd3582892440d2092c65df8472427566dbceaa9 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Wed, 27 Apr 2022 16:41:24 -0400

Currently debuginfod is disabled when the string of server URLs
is unset or set to be the empty string (via the $DEBUGINFOD_URLS
environment variable or the 'set debuginfod urls' gdb command).

Extend this functionality so that a whitespace-only URL also disables
debuginfod.

Modify a testcase to verify that a whitespace-only URL disables
debuginfod.
---
 gdb/debuginfod-support.c                        |  7 ++++---
 .../gdb.debuginfod/fetch_src_and_symbols.exp    | 17 +++++++++++++----
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 4ce2e786b6a..dffcd782e7d 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -173,10 +173,11 @@ get_debuginfod_client ()
 static bool
 debuginfod_is_enabled ()
 {
-  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  const char *urls = skip_spaces (getenv (DEBUGINFOD_URLS_ENV_VAR));
 
-  if (urls == nullptr || urls[0] == '\0'
-      || debuginfod_enabled == debuginfod_off)
+  if (debuginfod_enabled == debuginfod_off
+      || urls == nullptr
+      || *urls == '\0')
     return false;
 
   if (debuginfod_enabled == debuginfod_ask)
diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
index bd90bcd0cfe..74d026464b9 100644
--- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
+++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
@@ -309,7 +309,8 @@ proc local_url { } {
     clean_restart
     # Disable confirmation to avoid having to deal with a query.  See
     # test_urls.
-    gdb_test_multiple "with confirm off -- file $binfile" "notice empty URL" {
+    set file_cmd "with confirm off -- file $binfile"
+    gdb_test_multiple $file_cmd "notice empty URL" {
 	-re -wrap "This GDB supports auto-downloading.*" {
 	    fail $gdb_test_name
 	}
@@ -318,9 +319,17 @@ proc local_url { } {
 	}
     }
 
-    test_urls "   " \
-        "" \
-        "notice whitespace URL"
+    # Whitespace-only URLS disables Debuginfod.
+    setenv DEBUGINFOD_URLS "    "
+    clean_restart
+    gdb_test_multiple $file_cmd "notice whitespace URL" {
+	-re -wrap "This GDB supports auto-downloading.*" {
+	    fail $gdb_test_name
+	}
+	-re -wrap "" {
+	    pass $gdb_test_name
+	}
+    }
 
     set url "http://127.0.0.1:$port"
 
-- 
2.35.1

Re: [PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Pedro Alves]

On 2022-04-29 20:49, Aaron Merey wrote:
> On Fri, Apr 29, 2022 at 10:22 AM Pedro Alves <pedro@palves.net> wrote:
>> Does the rest of the function need to see "urls" with the leading space?  It seems like the 
>> code further below will also skip leading spaces anyhow.  IOW, we can't make this be: 
>>
>>     urls = skip_spaces (urls);
>>
>> or both lines replaced with:
>>
>>     const char *urls = skip_spaces (getenv (DEBUGINFOD_URLS_ENV_VAR));
>>
>> ?
> 
> I kept the "urls" with the leading whitespace so that we parse each URL 
> consistent with debuginfod's use of ' ' as the sole URL delimiter 
> (skip_space also skips tabs, newlines, etc).  However debuginfod severs
> should not have URLs containing unescaped whitespace so it's really a 
> moot point.
> 
> I've fixed this by combining skip_spaces and getenv on one line.

Thanks.  LGTM.

Re: [PATCH] gdb/debuginfod: Whitespace-only URL should disable debuginfod

[Aaron Merey]

On Mon, May 2, 2022 at 10:34 AM Pedro Alves <pedro@palves.net> wrote:
>
> On 2022-04-29 20:49, Aaron Merey wrote:
> >
> > I kept the "urls" with the leading whitespace so that we parse each URL
> > consistent with debuginfod's use of ' ' as the sole URL delimiter
> > (skip_space also skips tabs, newlines, etc).  However debuginfod severs
> > should not have URLs containing unescaped whitespace so it's really a
> > moot point.
> >
> > I've fixed this by combining skip_spaces and getenv on one line.
>
> Thanks.  LGTM.

Thanks, pushed as commit 95929abb49

Aaron