[v3,4/6] nto_process_target::create_inferior: Pass args as char **

Message ID 20200512154211.1311364-4-m.weghorn@posteo.de
State Superseded
Headers show
Series
  • [v3,1/6] gdb: Move construct_inferior_arguments to gdbsupport
Related show

Commit Message

Kevin Buettner via Gdb-patches May 12, 2020, 3:42 p.m.
Both, [1] and [2] suggest that the fifth parameter
to the 'spawnp' function is of type 'char * const argv[]',
so just pass the args contained in the vector as
an array right away, rather than converting that
to a C string first and passing that one.

With commit 2090129c36c7e582943b7d300968d19b46160d84
("Share fork_inferior et al with gdbserver",
2016-12-22) the type had changed from 'char **'
to 'char *', but I can't see an apparent reason for
that, and 'nto_procfs_target::create_inferior'
(in gdb/nto-procfs.c) also passes a 'char **' to
'spawnp' instead.

I do not know much about that target and cannot actually
test this, however.
The main motivation to look at this was identifying
and replacing the remaining uses of the 'stringify_argv'
function which does not properly do escaping.

[1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm
[2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm

gdbserver/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * nto-low.cc (nto_process_target::create_inferior): Pass
        argv to spawnp function as char **.
---
 gdbserver/nto-low.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.26.2

Comments

Simon Marchi May 12, 2020, 6:34 p.m. | #1
On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> Both, [1] and [2] suggest that the fifth parameter

> to the 'spawnp' function is of type 'char * const argv[]',

> so just pass the args contained in the vector as

> an array right away, rather than converting that

> to a C string first and passing that one.

> 

> With commit 2090129c36c7e582943b7d300968d19b46160d84

> ("Share fork_inferior et al with gdbserver",

> 2016-12-22) the type had changed from 'char **'

> to 'char *', but I can't see an apparent reason for

> that, and 'nto_procfs_target::create_inferior'

> (in gdb/nto-procfs.c) also passes a 'char **' to

> 'spawnp' instead.

> 

> I do not know much about that target and cannot actually

> test this, however.

> The main motivation to look at this was identifying

> and replacing the remaining uses of the 'stringify_argv'

> function which does not properly do escaping.

> 

> [1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm

> [2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm


I would suggest using this reference instead, which is from QNX Neutrino (the
platform this port is for).

http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html

Otherwise, this patch is OK.

Simon
Kevin Buettner via Gdb-patches May 13, 2020, 9:56 a.m. | #2
On 12/05/2020 20.34, Simon Marchi wrote:
> On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:

>> Both, [1] and [2] suggest that the fifth parameter

>> to the 'spawnp' function is of type 'char * const argv[]',

>> so just pass the args contained in the vector as

>> an array right away, rather than converting that

>> to a C string first and passing that one.

>>

>> With commit 2090129c36c7e582943b7d300968d19b46160d84

>> ("Share fork_inferior et al with gdbserver",

>> 2016-12-22) the type had changed from 'char **'

>> to 'char *', but I can't see an apparent reason for

>> that, and 'nto_procfs_target::create_inferior'

>> (in gdb/nto-procfs.c) also passes a 'char **' to

>> 'spawnp' instead.

>>

>> I do not know much about that target and cannot actually

>> test this, however.

>> The main motivation to look at this was identifying

>> and replacing the remaining uses of the 'stringify_argv'

>> function which does not properly do escaping.

>>

>> [1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm

>> [2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm

> 

> I would suggest using this reference instead, which is from QNX Neutrino (the

> platform this port is for).

> 

> http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html

> 

> Otherwise, this patch is OK.


Thanks for the hint, I have update the commit message for v4 of the patch.

Michael

Patch

diff --git a/gdbserver/nto-low.cc b/gdbserver/nto-low.cc
index 642fe9ffd2..a88ad02f64 100644
--- a/gdbserver/nto-low.cc
+++ b/gdbserver/nto-low.cc
@@ -357,7 +357,6 @@  nto_process_target::create_inferior (const char *program,
   struct inheritance inherit;
   pid_t pid;
   sigset_t set;
-  std::string str_program_args = stringify_argv (program_args);
 
   TRACE ("%s %s\n", __func__, program);
   /* Clear any pending SIGUSR1's but keep the behavior the same.  */
@@ -371,7 +370,7 @@  nto_process_target::create_inferior (const char *program,
   inherit.flags |= SPAWN_SETGROUP | SPAWN_HOLD;
   inherit.pgroup = SPAWN_NEWPGROUP;
   pid = spawnp (program, 0, NULL, &inherit,
-		(char *) str_program_args.c_str (), 0);
+		program_args.data (), 0);
   sigprocmask (SIG_BLOCK, &set, NULL);
 
   if (pid == -1)