On Monday 10 November 2014 09:25:13 Daniel Stenberg wrote:
> On Thu, 6 Nov 2014, Peter Wu wrote:
>
> > This prepares for a future patch where commands are not executed by a shell
> > anymore, but by perl. In the future, Text::ParseWords will be used to
> > convert a command to an array.
>
> I prefer to bundle this change with this "future patch" you're talking about.
> Especially since we don't do anything to prevent new double-quotes to be
> introduced with the same use case.

New double quotes is not an issue, except when it contains backslashes (or other
special characters such as backticks and dollars for variables).

> Is Text::ParseWords a module that is installed by default with perl? I'm
> hesitant to require any addditional stuff for perl.

Text::ParseWords is a core module, but the future patch will depend on IPC::Run
which is not in core. Arch Linux has it in its main repositories though as does
Debian (and by extension, Ubuntu). Since this IPC::Run module is already proven,
I would like to reuse this rather than writing a new implementation or copying
it in-tree.

> Also, what is the benefit of letting perl do this and not the shell? Are you
> planning to make the test suite able to run completely without a shell? If so,
> what are the benefits and the drawbacks?

The motivation is outlined in the commit messages[1][2]. Basically, setting
LD_PRELOAD=libhostname.so always fails when curl is built with Address
Sanitizer[3] (and other sanitizers). This happens because a symbol is missing
when the library is dynamically loaded into sh (which is called due to
system("some command").

I had something like system("var1=val1 var2=val2 some command") in mind, but
when I asked in #perl about this, it became apparent that invoking shell
commands directly is suboptimal (phrased nicely) and that perl can by itself
also split the string into a command and arguments array and invoke that without
relying on a shell.

I have no plans to make the test suite run without shell, but to reduce the
unnecessary uses of it.

Advantages / disadvantages of the new patch:
 + Slightly faster in theory as no shell needs to be invoked per test.
 + Ability to use LD_PRELOAD on ASAN-enabled binaries.
 + The ability to build commands safely without having special characters in
   variables break the command. Think of spaces, dollars and backslashes.
   Note that this is not implemented yet, but it can be if there is need for it.
 - Less flexibility as you cannot use shell constructs anymore (subcommands
   through backticks).
 - Depends on an external IPC::Run module (while commonly available, it is not
   in core).

-- 
Kind regards,
Peter
https://lekensteyn.nl
 [1]: https://github.com/Lekensteyn/curl/commit/85db455
 [2]: https://github.com/Lekensteyn/curl/commit/55850b6b
 [3]: https://code.google.com/p/address-sanitizer/wiki/AddressSanitizer
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html