command_line_input() not re-entrant

command_line_input() not re-entrant

[Andrew STUBBS]

Hi,

I have discovered a problem in the GDB command line reading code.

command_line_input() uses a static buffer to hold the current command. 
This means that it is not properly re-entrant - commands that contain 
other commands, such as user defined commands, are not handled safely.

In practice the only real trouble I have observed is with user defined 
commands that use $arg0 etc. because these parameters are never copied 
out of the original string, so are overwritten the next time 
command_line_input() is invoked. Even then, this is not normally a 
problem because command_line_input() is not normally needed within a 
user-defined command - it has already been read. It is only a problem 
when the user defined command contains a source command.

The problem may be reproduced as follows:

Create three files:

a1
---8<---------->8-----
source a2
abcdef qwerty
---8<---------->8-----

a2
---8<---------->8-----
define abcdef
   echo 1: <<<$arg0>>>\n
   source a3
   echo 2: <<<$arg0>>>\n
end
---8<---------->8-----

a3
---8<---------->8-----
#################################################################
---8<---------->8-----

Then run the following command:

$ gdb -nx -q -x a1 -batch
1: <<<qwerty>>>
2: <<<######>>>

Both 1: and 2: should have been the same. As you can see the contents of 
a3 have overwritten the value of $arg0 in abcdef. For some reason I 
haven't discovered (and probably boils down to dumb luck) I can't 
reproduce the problem when entering a1 interactively - I have to source it.

I am happy to write the patch to fix this but I am wondering how. There 
seem to be two possible ways:

1. Make command_line_input() re-entrant. Perhaps drop the static buffer 
and malloc a new string each time. Free it through a clean-up.

2. Have setup_user_args() copy the data and adjust the clean up to free 
the copied data.

Any preferences or other suggestions?

Andrew Stubbs

[PATCH] allow nested sourced commands

[Andrew STUBBS]

Andrew Stubbs wrote:
> I have discovered a problem in the GDB command line reading code.
> 
> command_line_input() uses a static buffer to hold the current command. 
> This means that it is not properly re-entrant - commands that contain 
> other commands, such as user defined commands, are not handled safely.

See http://sources.redhat.com/ml/gdb-patches/2006-03/msg00356.html

The attached patch should fix the problem.

I tried to fix the problem in command_line_input, but there were too 
many ways for the string to leak, so I have opted for the simpler fix, 
even though it feels like treating the symptoms, not the problem.

Anyway, with this patch it no longer attempts to read data that has been 
overwritten, so everything works fine. Valgrind reports no problems with
the test case I posted before.

Andrew Stubbs

Re: [PATCH] allow nested sourced commands

[Andrew STUBBS]

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

Sorry, I forgot to attach the patch.

Andrew Stubbs wrote:
> Andrew Stubbs wrote:
>> I have discovered a problem in the GDB command line reading code.
>>
>> command_line_input() uses a static buffer to hold the current command. 
>> This means that it is not properly re-entrant - commands that contain 
>> other commands, such as user defined commands, are not handled safely.
> 
> See http://sources.redhat.com/ml/gdb-patches/2006-03/msg00356.html
> 
> The attached patch should fix the problem.
> 
> I tried to fix the problem in command_line_input, but there were too 
> many ways for the string to leak, so I have opted for the simpler fix, 
> even though it feels like treating the symptoms, not the problem.
> 
> Anyway, with this patch it no longer attempts to read data that has been 
> overwritten, so everything works fine. Valgrind reports no problems with
> the test case I posted before.
> 
> Andrew Stubbs
> 


[-- Attachment #2: reentrant-commands.patch --]
[-- Type: text/plain, Size: 959 bytes --]

2006-04-04  Andrew Stubbs  <andrew.stubbs@st.com>

	* cli/cli-script.c (struct user_args): Add command field.
	(arg_cleanup): Free command string.
	(setup_user_args): Copy the command line before relying on it.

Index: src/gdb/cli/cli-script.c
===================================================================
--- src.orig/gdb/cli/cli-script.c	2006-04-04 10:53:26.000000000 +0100
+++ src/gdb/cli/cli-script.c	2006-04-04 11:09:33.000000000 +0100
@@ -54,6 +54,7 @@ static int control_level;
 struct user_args
   {
     struct user_args *next;
+    char *command;
     struct
       {
 	char *arg;
@@ -483,6 +484,7 @@ arg_cleanup (void *ignore)
 		    _("arg_cleanup called with no user args.\n"));
 
   user_args = user_args->next;
+  xfree (oargs->command);
   xfree (oargs);
 }
 
@@ -507,6 +509,8 @@ setup_user_args (char *p)
   if (p == NULL)
     return old_chain;
 
+  user_args->command = p = xstrdup (p);
+
   while (*p)
     {
       char *start_arg;

Re: [PATCH] allow nested sourced commands

[Michael Snyder]

Andrew STUBBS wrote:
>>
>>> I have discovered a problem in the GDB command line reading code.
>>>
>>> command_line_input() uses a static buffer to hold the current 
>>> command. This means that it is not properly re-entrant - commands 
>>> that contain other commands, such as user defined commands, are not 
>>> handled safely.
>>
>>
>> See http://sources.redhat.com/ml/gdb-patches/2006-03/msg00356.html
>>
>> The attached patch should fix the problem.
>>
>> I tried to fix the problem in command_line_input, but there were too 
>> many ways for the string to leak, so I have opted for the simpler fix, 
>> even though it feels like treating the symptoms, not the problem.
>>
>> Anyway, with this patch it no longer attempts to read data that has 
>> been overwritten, so everything works fine. Valgrind reports no 
>> problems with
>> the test case I posted before.
>>
>> Andrew Stubbs

Well, it has the virtue of simplicity!
At first glance, it seems conceptually valid.

I think you need a clean-up, though.  What if it errors?


> ------------------------------------------------------------------------
> 
> 2006-04-04  Andrew Stubbs  <andrew.stubbs@st.com>
> 
> 	* cli/cli-script.c (struct user_args): Add command field.
> 	(arg_cleanup): Free command string.
> 	(setup_user_args): Copy the command line before relying on it.
> 
> Index: src/gdb/cli/cli-script.c
> ===================================================================
> --- src.orig/gdb/cli/cli-script.c	2006-04-04 10:53:26.000000000 +0100
> +++ src/gdb/cli/cli-script.c	2006-04-04 11:09:33.000000000 +0100
> @@ -54,6 +54,7 @@ static int control_level;
>  struct user_args
>    {
>      struct user_args *next;
> +    char *command;
>      struct
>        {
>  	char *arg;
> @@ -483,6 +484,7 @@ arg_cleanup (void *ignore)
>  		    _("arg_cleanup called with no user args.\n"));
>  
>    user_args = user_args->next;
> +  xfree (oargs->command);
>    xfree (oargs);
>  }
>  
> @@ -507,6 +509,8 @@ setup_user_args (char *p)
>    if (p == NULL)
>      return old_chain;
>  
> +  user_args->command = p = xstrdup (p);
> +
>    while (*p)
>      {
>        char *start_arg;

Re: [PATCH] allow nested sourced commands

[Andrew STUBBS]

Michael Snyder wrote:
> Well, it has the virtue of simplicity!
> At first glance, it seems conceptually valid.
> 
> I think you need a clean-up, though.  What if it errors?

Look again, more closely :) arg_cleanup IS a clean-up.

Does that mean it is OK?

Thanks for the quick review

Andrew

Re: [PATCH] allow nested sourced commands

[Michael Snyder]

Andrew STUBBS wrote:
> Michael Snyder wrote:
> 
>> Well, it has the virtue of simplicity!
>> At first glance, it seems conceptually valid.
>>
>> I think you need a clean-up, though.  What if it errors?
> 
> Look again, more closely :) arg_cleanup IS a clean-up.

Ah.  So it is.  ;-)

> Does that mean it is OK?

Hmmm, well, I'm not too familiar with that code, but it looks OK
programatically.  Certainly should be commented better, though,
esp. since a struct called "user_args" now contains something
that's not a user arg.

Can you add a test in the testsuite?

Re: [PATCH] allow nested sourced commands

[Andrew STUBBS]

Michael Snyder wrote:
> Hmmm, well, I'm not too familiar with that code, but it looks OK
> programatically.  Certainly should be commented better, though,
> esp. since a struct called "user_args" now contains something
> that's not a user arg.

Ok, fair enough. How about the following inserted into the struct 
definition:

/* It is necessary to store a malloced copy of the command line to
ensure that the arguments are not overwritten before they are used.  */

> Can you add a test in the testsuite?

Yes, I have the test case I posted before. Where would be the correct 
place to add the test? Just add a new file under gdb.base?

Andrew

Re: [PATCH] allow nested sourced commands

[Daniel Jacobowitz]

On Thu, Apr 06, 2006 at 10:46:09AM +0100, Andrew STUBBS wrote:
> /* It is necessary to store a malloced copy of the command line to
> ensure that the arguments are not overwritten before they are used.  */

Sounds good to me.

> >Can you add a test in the testsuite?
> 
> Yes, I have the test case I posted before. Where would be the correct 
> place to add the test? Just add a new file under gdb.base?

Yes, please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] allow nested sourced commands

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
>> Yes, I have the test case I posted before. Where would be the correct 
>> place to add the test? Just add a new file under gdb.base?
> 
> Yes, please.

On closer inspection it would appear that there are already similar 
tests in commands.exp, so I have added it there.

Here is the entire patch. I have tested the testcase with and without my 
fix - it seems to work correctly.

OK?

Andrew Stubbs

[-- Attachment #2: reentrant-commands.patch --]
[-- Type: text/plain, Size: 2410 bytes --]

2006-04-07  Andrew Stubbs  <andrew.stubbs@st.com>

gdb/
	* cli/cli-script.c (struct user_args): Add command field.
	(arg_cleanup): Free command string.
	(setup_user_args): Copy the command line before relying on it.

gdb/testsuite/
	* gdb.base/commands.exp (recursive_source_test): New test.

Index: src/gdb/cli/cli-script.c
===================================================================
--- src.orig/gdb/cli/cli-script.c	2006-04-07 12:01:55.000000000 +0100
+++ src/gdb/cli/cli-script.c	2006-04-07 12:12:03.000000000 +0100
@@ -54,6 +54,7 @@ static int control_level;
 struct user_args
   {
     struct user_args *next;
+    char *command;
     struct
       {
 	char *arg;
@@ -483,6 +484,7 @@ arg_cleanup (void *ignore)
 		    _("arg_cleanup called with no user args.\n"));
 
   user_args = user_args->next;
+  xfree (oargs->command);
   xfree (oargs);
 }
 
@@ -507,6 +509,8 @@ setup_user_args (char *p)
   if (p == NULL)
     return old_chain;
 
+  user_args->command = p = xstrdup (p);
+
   while (*p)
     {
       char *start_arg;
Index: src/gdb/testsuite/gdb.base/commands.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/commands.exp	2006-04-07 12:11:50.000000000 +0100
+++ src/gdb/testsuite/gdb.base/commands.exp	2006-04-07 12:10:02.000000000 +0100
@@ -583,7 +583,40 @@ proc stray_arg0_test { } {
 	"\\\$\[0-9\]* = 1" \
 	"stray_arg0_test #4"
 }
-  
+
+# Test that GDB can handle arguments when sourcing files recursively.
+# If the arguments are overwritten with ####### then the test has failed.
+proc recursive_source_test {} {
+    set fd [open "file1" w]
+    puts $fd \
+{source file2
+abcdef qwerty}
+    close $fd
+
+    set fd [open "file2" w]
+    puts $fd \
+{define abcdef
+  echo 1: <<<$arg0>>>\n
+  source file3
+  echo 2: <<<$arg0>>>\n
+end}
+    close $fd
+
+    set fd [open "file3" w]
+    puts $fd \
+"echo in file3\\n
+#################################################################"
+    close $fd
+
+    gdb_test "source file1" \
+	"1: <<<qwerty>>>\[\r\n]+in file3\[\r\n]+2: <<<qwerty>>>" \
+	"recursive source test"
+
+    file delete file1
+    file delete file2
+    file delete file3
+}
+
 gdbvar_simple_if_test
 gdbvar_simple_while_test
 gdbvar_complex_if_while_test
@@ -600,3 +633,4 @@ deprecated_command_test
 bp_deleted_in_command_test
 temporary_breakpoint_commands
 stray_arg0_test
+recursive_source_test

Re: [PATCH] allow nested sourced commands

[Daniel Jacobowitz]

On Fri, Apr 07, 2006 at 12:15:04PM +0100, Andrew STUBBS wrote:
> Here is the entire patch. I have tested the testcase with and without my 
> fix - it seems to work correctly.
> 
> OK?

This is fine, except you lost the explanatory comment.  OK with that
readded.  Thanks!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] allow nested sourced commands

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> This is fine, except you lost the explanatory comment.  OK with that
> readded.  Thanks!

Quite right, silly mistake.

I've committed the attached.

Andrew

[-- Attachment #2: reentrant-commands.patch --]
[-- Type: text/plain, Size: 2562 bytes --]

2006-04-07  Andrew Stubbs  <andrew.stubbs@st.com>

gdb/
	* cli/cli-script.c (struct user_args): Add command field.
	(arg_cleanup): Free command string.
	(setup_user_args): Copy the command line before relying on it.

gdb/testsuite/
	* gdb.base/commands.exp (recursive_source_test): New test.

Index: src/gdb/cli/cli-script.c
===================================================================
--- src.orig/gdb/cli/cli-script.c	2006-04-07 12:01:55.000000000 +0100
+++ src/gdb/cli/cli-script.c	2006-04-07 14:25:58.000000000 +0100
@@ -54,6 +54,9 @@ static int control_level;
 struct user_args
   {
     struct user_args *next;
+    /* It is necessary to store a malloced copy of the command line to
+       ensure that the arguments are not overwritten before they are used.  */
+    char *command;
     struct
       {
 	char *arg;
@@ -483,6 +486,7 @@ arg_cleanup (void *ignore)
 		    _("arg_cleanup called with no user args.\n"));
 
   user_args = user_args->next;
+  xfree (oargs->command);
   xfree (oargs);
 }
 
@@ -507,6 +511,8 @@ setup_user_args (char *p)
   if (p == NULL)
     return old_chain;
 
+  user_args->command = p = xstrdup (p);
+
   while (*p)
     {
       char *start_arg;
Index: src/gdb/testsuite/gdb.base/commands.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/commands.exp	2006-04-07 12:11:50.000000000 +0100
+++ src/gdb/testsuite/gdb.base/commands.exp	2006-04-07 12:10:02.000000000 +0100
@@ -583,7 +583,40 @@ proc stray_arg0_test { } {
 	"\\\$\[0-9\]* = 1" \
 	"stray_arg0_test #4"
 }
-  
+
+# Test that GDB can handle arguments when sourcing files recursively.
+# If the arguments are overwritten with ####### then the test has failed.
+proc recursive_source_test {} {
+    set fd [open "file1" w]
+    puts $fd \
+{source file2
+abcdef qwerty}
+    close $fd
+
+    set fd [open "file2" w]
+    puts $fd \
+{define abcdef
+  echo 1: <<<$arg0>>>\n
+  source file3
+  echo 2: <<<$arg0>>>\n
+end}
+    close $fd
+
+    set fd [open "file3" w]
+    puts $fd \
+"echo in file3\\n
+#################################################################"
+    close $fd
+
+    gdb_test "source file1" \
+	"1: <<<qwerty>>>\[\r\n]+in file3\[\r\n]+2: <<<qwerty>>>" \
+	"recursive source test"
+
+    file delete file1
+    file delete file2
+    file delete file3
+}
+
 gdbvar_simple_if_test
 gdbvar_simple_while_test
 gdbvar_complex_if_while_test
@@ -600,3 +633,4 @@ deprecated_command_test
 bp_deleted_in_command_test
 temporary_breakpoint_commands
 stray_arg0_test
+recursive_source_test