[PATCH] Don't split executable paths with spaces in into multiple arguments

[PATCH] Don't split executable paths with spaces in into multiple arguments

[Jon Beniston]

Hi,

The following patch prevents executable paths with spaces in them from being
passed to the simulator as multiple arguments.

OK to apply?

Cheers,
Jon

gdb/
2009-11-04  Jon Beniston  <jon@beniston.com>

	* remote-sim.c(gdbsim_create_inferior) Quote executable path in case
it
	contains spaces.

===================================================================
--- remote-sim.c        (revision 41)
+++ remote-sim.c        (working copy)
@@ -458,10 +458,13 @@

   if (exec_file != NULL)
     {
-      len = strlen (exec_file) + 1 + strlen (args) + 1 + /*slop */ 10;
+      len = 2 + strlen (exec_file) + 1 + strlen (args) + 1 + /*slop */ 10;
       arg_buf = (char *) alloca (len);
       arg_buf[0] = '\0';
+      /* Add quotes around exec_file in case there are spaces in the path.
*/
+      strcat (arg_buf, "\"");
       strcat (arg_buf, exec_file);
+      strcat (arg_buf, "\"");
       strcat (arg_buf, " ");
       strcat (arg_buf, args);
       argv = buildargv (arg_buf);

Re: [PATCH] Don't split executable paths with spaces in into multiple arguments

[Joel Brobecker]

> gdb/
> 2009-11-04  Jon Beniston  <jon@beniston.com>
> 
> 	* remote-sim.c(gdbsim_create_inferior) Quote executable path in case
> it
> 	contains spaces.

OK.  This is not handling the case where the filename contains a double
quote, but it is not making things worse, and double-quotes in filenames
are probably extremely rare.

Just a tiny formatting request: Please add a space before
"(gdbsim_create_inferior)" and make sure that each line does not
exceed 80 character.

> -      len = strlen (exec_file) + 1 + strlen (args) + 1 + /*slop */ 10;
> +      len = 2 + strlen (exec_file) + 1 + strlen (args) + 1 + /*slop */ 10;

This is not your doing, but the remote-sim code is full of "slop" and
"slack", etc, and I wonder how hard it would be to make it crash due
to not enough "slack" :-(.

-- 
Joel

Re: [PATCH] Don't split executable paths with spaces in into multiple arguments

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> gdb/
>> 2009-11-04  Jon Beniston  <jon@beniston.com>
>> * remote-sim.c(gdbsim_create_inferior) Quote executable path in case
>> it
>> contains spaces.

Joel> OK.  This is not handling the case where the filename contains a double
Joel> quote, but it is not making things worse, and double-quotes in filenames
Joel> are probably extremely rare.

It seems to me that it would not be significantly harder for the code to
do the correct thing in all cases.

Tom

Re: [PATCH] Don't split executable paths with spaces in into multiple arguments

[Joel Brobecker]

> It seems to me that it would not be significantly harder for the code to
> do the correct thing in all cases.

To be honest, I didn't even think about it, mostly because I thought
that the issue would be so rare that it was possibly just retorical.
So I felt that I shouldn't insist that we do the right thing in all
cases.  Too lax?

-- 
Joel

Re: [PATCH] Don't split executable paths with spaces in into multiple arguments

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> It seems to me that it would not be significantly harder for the code to
>> do the correct thing in all cases.

Joel> To be honest, I didn't even think about it, mostly because I thought
Joel> that the issue would be so rare that it was possibly just retorical.
Joel> So I felt that I shouldn't insist that we do the right thing in all
Joel> cases.  Too lax?

Ordinarily my view is "forward biased": an improvement is an
improvement, and requiring perfection tends to raise the patch
submission bar too high.

In this particular case, though, I would probably have pushed back a
bit, first because the patch is already fixing a previous "can't happen"
assumption, and second because the correct code is really just 4 or 5
lines.  (I thought for sure we would already have the quoting code lying
around, but I grepped a little and couldn't find it.  It is odd that we
have buildargv to split the argv but nothing to quote it.)

Tom

Re: [PATCH] Don't split executable paths with spaces in into multiple arguments

[Joel Brobecker]

> In this particular case, though, I would probably have pushed back a
> bit, first because the patch is already fixing a previous "can't happen"
> assumption, and second because the correct code is really just 4 or 5
> lines.  (I thought for sure we would already have the quoting code lying
> around, but I grepped a little and couldn't find it.  It is odd that we
> have buildargv to split the argv but nothing to quote it.)

I understand.

Jon,

Would you mind enhancing your patch to handle all cases properly,
please?

Thanks,
-- 
Joel