From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 9F9E7385DC1B for ; Thu, 7 May 2020 20:09:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9F9E7385DC1B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BF0F11E581; Thu, 7 May 2020 16:09:20 -0400 (EDT) Subject: Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars To: Michael Weghorn , gdb-patches@sourceware.org References: <20200429111638.1327262-1-m.weghorn@posteo.de> <20200429111638.1327262-5-m.weghorn@posteo.de> From: Simon Marchi Message-ID: <17f345c8-eb7f-a3bb-c133-98533c7fa1c5@simark.ca> Date: Thu, 7 May 2020 16:09:20 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200429111638.1327262-5-m.weghorn@posteo.de> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 May 2020 20:09:26 -0000 On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote: > Use the construct_inferior_arguments function instead of > stringify_argv to construct a string from the program > arguments in those places where that one is then passed > to fork_inferior, since construct_inferior_arguments > properly takes care of special characters, while > stringify_argv does not. > Using construct_inferior_arguments seems "natural", since its > documentation also mentions that it "does the > same shell processing as fork_inferior". > > This makes gdbserver properly handle program args containing special > characters, e.g. (example from PR25893) > > $ gdbserver localhost:50505 myprogram "hello world" > > now properly handles "hello world" as a single arg, not two separate > ones ("hello", "world"). > > I'm not sure regarding the two remaining uses of function stringify_argv > (in win32_process_target::create_inferior and > nto_process_target::create_inferior) whose result is not passed to > fork_inferior and don't know how the arg processing in the creation > of processes on those targets exactly works, so left them unchanged > for now. I'd say, do the change the best you can for win32 and nto. We'll be able to test win32. Regarding nto, it hasn't seen patches for years, it most likely doesn't even build. And the toolchain not being easily available, we can't do much about it. I think we should actually consider removing this port, unless somebody steps up to maintain it. Same for lynx. This will allow removing stringify_argv, and avoid having duplicate broken functionality of construct_inferior_arguments. > 2020-04-29 Michael Weghorn > > PR gdbserver/25893 > * linux-low.cc (linux_process_target::create_inferior), > lynx-low.cc (lynx_process_target::create_inferior): Use > construct_inferior_arguments instead of stringify_argv > to get string representation which properly escapes > special characters. > --- > gdbserver/linux-low.cc | 2 +- > gdbserver/lynx-low.cc | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc > index 3cd8d5594d..3f3ffc6c43 100644 > --- a/gdbserver/linux-low.cc > +++ b/gdbserver/linux-low.cc > @@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program, > { > maybe_disable_address_space_randomization restore_personality > (cs.disable_randomization); > - std::string str_program_args = stringify_argv (program_args); > + std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data()); This line is too long, see below. > > pid = fork_inferior (program, > str_program_args.c_str (), > diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc > index 9aa140c129..61f692f0c0 100644 > --- a/gdbserver/lynx-low.cc > +++ b/gdbserver/lynx-low.cc > @@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program, > const std::vector &program_args) > { > int pid; > - std::string str_program_args = stringify_argv (program_args); > + std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());; Extra semi-colon, and spaces before parenthesis. The line should fit in 80 columns, here's how we'd typically break it: std::string str_program_args = construct_inferior_arguments (program_args.size (), program_args.data ()); (with the equal sign on the second line, I know that may seem odd) If you are game, you could make another preparatory patch (anywhere in the series before this one) that would make construct_inferior_arguments take a gdb::array_view parameter. That would allow you to call it more simply here: construct_inferior_arguments (program_args); Simon