diff mbox series

[1/1] gdb: Fix null pointer dereference on missing PATH variable

Message ID 20250304215011.2092-1-daniel-email@gmx.net
State New
Headers
Series [1/1] gdb: Fix null pointer dereference on missing PATH variable |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Daniel Starke March 4, 2025, 9:50 p.m. UTC 
  When running "show" with missing PATH variable a null pointer is being
dereferenced in path_info().

path_command() correctly checks whether PATH has been set before using it.
It then calls path_info() which retrieves the variable again but fails to
perform the null pointer test on it. As a result, the application crashes with
SIGSEGV on Windows for example.

Fix this by handling the null pointer case in path_info() accordingly.

Signed-off-by: Daniel Starke <daniel-email@gmx.net>
---
 gdb/infcmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.39.5

Comments

Simon Marchi March 5, 2025, 3:38 a.m. UTC | #1 
On 2025-03-04 16:50, Daniel Starke wrote:
> When running "show" with missing PATH variable a null pointer is being
> dereferenced in path_info().
> 
> path_command() correctly checks whether PATH has been set before using it.
> It then calls path_info() which retrieves the variable again but fails to
> perform the null pointer test on it. As a result, the application crashes with
> SIGSEGV on Windows for example.
> 
> Fix this by handling the null pointer case in path_info() accordingly.
> 
> Signed-off-by: Daniel Starke <daniel-email@gmx.net>
> ---
>  gdb/infcmd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 00703e44b7b..adb2592ae8e 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2116,7 +2116,10 @@ static void
>  path_info (const char *args, int from_tty)
>  {
>    gdb_puts ("Executable and object file path: ");
> -  gdb_puts (current_inferior ()->environment.get (path_var_name));
> +  const char *env = current_inferior ()->environment.get (path_var_name);
> +  if (!env)
> +    env = "";
> +  gdb_puts (env);
>    gdb_puts ("\n");
>  }
> 
> --
> 2.39.5
> 

I was wondering why I couldn't reproduce on Linux.  On my system, the
gdb_puts call goes to pager_file::puts, which does handle the nullptr
case:

  if (linebuffer == 0)
    return;

On Windows, I suppose it goes to stdio_file::puts directly or something
like that, which doesn't handle the nullptr case.

I propose this little tweak shown below, changing the code to use
gdb_printf instead of gdb_puts, if that's ok with you.


From 37542545e29b2f5c2b8d1defcfa37e3e3b921854 Mon Sep 17 00:00:00 2001
From: Daniel Starke <daniel-email@gmx.net>
Date: Tue, 4 Mar 2025 22:50:11 +0100
Subject: [PATCH] gdb: fix null pointer dereference on missing PATH variable

When running "show" with missing PATH variable a null pointer is being
dereferenced in path_info().

path_command() correctly checks whether PATH has been set before using it.
It then calls path_info() which retrieves the variable again but fails to
perform the null pointer test on it. As a result, the application crashes with
SIGSEGV on Windows for example.

Fix this by handling the null pointer case in path_info() accordingly.

Signed-off-by: Daniel Starke <daniel-email@gmx.net>
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I41ef10f00802d3163793491454190008e78f5dc1
---
 gdb/infcmd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 00703e44b7b5..06b7038df506 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2115,9 +2115,10 @@ static const char path_var_name[] = "PATH";
 static void
 path_info (const char *args, int from_tty)
 {
-  gdb_puts ("Executable and object file path: ");
-  gdb_puts (current_inferior ()->environment.get (path_var_name));
-  gdb_puts ("\n");
+  const char *env = current_inferior ()->environment.get (path_var_name);
+
+  gdb_printf ("Executable and object file path: %s\n",
+	      env != nullptr ? env : "");
 }
 
 /* Add zero or more directories to the front of the execution path.  */

base-commit: aa2cd0e39dc81b28ba7c934faac18bd4d8287450
Tom Tromey March 5, 2025, 3:28 p.m. UTC | #2 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I propose this little tweak shown below, changing the code to use
Simon> gdb_printf instead of gdb_puts, if that's ok with you.

Simon> +  gdb_printf ("Executable and object file path: %s\n",

Probably should use _() here.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Simon Marchi March 5, 2025, 3:51 p.m. UTC | #3 
On 3/5/25 10:28 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> I propose this little tweak shown below, changing the code to use
> Simon> gdb_printf instead of gdb_puts, if that's ok with you.
> 
> Simon> +  gdb_printf ("Executable and object file path: %s\n",
> 
> Probably should use _() here.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks, pushing with that fixed.

Simon
Daniel Starke March 5, 2025, 4:17 p.m. UTC | #4 
> I propose this little tweak shown below, changing the code to use
> gdb_printf instead of gdb_puts, if that's ok with you.
I did not test that but sure.
diff mbox series

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 00703e44b7b..adb2592ae8e 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2116,7 +2116,10 @@  static void
 path_info (const char *args, int from_tty)
 {
   gdb_puts ("Executable and object file path: ");
-  gdb_puts (current_inferior ()->environment.get (path_var_name));
+  const char *env = current_inferior ()->environment.get (path_var_name);
+  if (!env)
+    env = "";
+  gdb_puts (env);
   gdb_puts ("\n");
 }