cURL / Mailing Lists / curl-library / Single Mail

curl-library

[PATCH 1/2] runtests.pl: safer system() calls, fix LD_PRELOAD

From: Peter Wu <peter_at_lekensteyn.nl>
Date: Sat, 1 Nov 2014 22:44:04 +0100

Command invocation via runclient() (which calls system()) using a string
invokes the shell. This has the problem that tests which load
libhostname.so via LD_PRELOAD break when the library cannot be loaded
into the shell (when cURL is built with AddressSanitizer for example).

This patch removes the ability to use shell constructs in commands, such
as backticks, but fixes the above case where an ASAN-enabled library is
loaded with LD_PRELOAD.

The system call (in runclient) and backticks operator (for precheck) are
replaced by fork/open/exec to avoid executing sh. Note that sh is still
invoked when curl is built with shared libs in autotools due to the
libtool wrapper script.

Note for the curious: previously a gdb invocation would feed stdin via
'set args <input'. This only works because the command is interpreted as
a shell command. After this patch stdin is fed directly.

Detection of the `valgrind --tool` option was previously done by
invoking grep. Remove this use of grep. While at it, remove
`valgrind --version` invocation to determine whether `--log-file`
should be used instead of `--logfile`. Parse this information directly
from `valgrind --help` which was executed earlier.

As for differences in log file, these are just whitespace (trailing
space for `curl --version` command, spaces after redirection operator)
and quoting (quotes typically get stripped).

Signed-off-by: Peter Wu <peter_at_lekensteyn.nl>

---
 tests/runtests.pl | 380 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 261 insertions(+), 119 deletions(-)
diff --git a/tests/runtests.pl b/tests/runtests.pl
index 8802c0c..b465ae9 100755
--- a/tests/runtests.pl
+++ b/tests/runtests.pl
@@ -69,6 +69,7 @@ BEGIN {
 use strict;
 use warnings;
 use Cwd;
+use File::Spec;
 
 # Subs imported from serverhelp module
 use serverhelp qw(
@@ -502,45 +503,111 @@ sub checktestcmd {
 #######################################################################
 # Run the application under test and return its return code
 #
+# A hash is expected as parameter. Keys:
+# - cmd is a reference to an array.
+# - stdin is an optional filename.
+# - stdout and stderr are either strings (a filename or '&1' for stdout) or
+#   a reference to store the output. Note: stdout is always redirected before
+#   stderr.
+#
 sub runclient {
-    my ($cmd)=@_;
-    my $ret = system($cmd);
-    print "CMD ($ret): $cmd\n" if($verbose && !$torture);
-    return $ret;
+    my %args = @_;
+    # To test curl on a remote machine, replace the command by
+    # 'ssh $CLIENTIP "cd $PWD && $cmd', but note shell escaping and take care of
+    # env vars. Then sleep(2) to allow the NFS server to be updated.
+    my $pid = fork();
+    if (!defined($pid)) {
+        print "fork failed with $@\n" if($verbose && !$torture);
+        return -1;
+    }
+    if ($pid == 0) { # child
+        open(STDIN, '<', $args{stdin}) if $args{stdin};
+        my %outputs = ( 'stdout' => *STDOUT, 'stderr' => *STDERR );
+        while (my ($key, $handle) = each %outputs) {
+            if (my $dest = $args{$key}) {
+                if ($dest =~ /^&/) {
+                    open($handle, ">$dest");
+                } else {
+                    # Scalar or other file name (do not treat "-" specially!)
+                    open($handle, '>', $dest);
+                }
+            }
+        }
+        # Try to invoke the command or exit
+        exec(@{$args{cmd}}) or exit(127);
+    }
 
-# This is one way to test curl on a remote machine
-#    my $out = system("ssh $CLIENTIP cd \'$pwd\' \\; \'$cmd\'");
-#    sleep 2;    # time to allow the NFS server to be updated
-#    return $out;
+    # Parent
+    waitpid($pid, 0);
+    my $ret = $?;
+    print "CMD ($ret): " . cmd_tostring(%args) . "\n" if($verbose && !$torture);
+    return $ret;
 }
 
 #######################################################################
-# Run the application under test and return its stdout
+# Converts the options (that can be passed to runclient) to a string.
 #
-sub runclientoutput {
-    my ($cmd)=@_;
-    return `$cmd`;
+sub cmd_tostring {
+    my %args = @_;
+    my @cmd;
+    push @cmd, "'" . s/'/'\\''/gr . "'" for (@{$args{cmd}});
+    # when given, stdin can only be a filename.
+    push @cmd, "<$args{stdin}" if $args{stdin};
+    # Print redir op only if redirection is redirected which is a filename (and
+    # not a reference to an output variable).
+    push @cmd,  ">$args{stdout}" if ($args{stdout} and !ref($args{stdout}));
+    push @cmd, "2>$args{stderr}" if ($args{stderr} and !ref($args{stderr}));
+    return "@cmd";
+}
 
-# This is one way to test curl on a remote machine
-#    my @out = `ssh $CLIENTIP cd \'$pwd\' \\; \'$cmd\'`;
-#    sleep 2;    # time to allow the NFS server to be updated
-#    return @out;
- }
+# Splits a shell command into an array that can be passed to exec.
+sub splitcmd {
+    my $cmdline = shift;
+    my @args;
+    my $arg;
+    # Match all arguments, removing quotes/backslashes as needed.
+    # Text::ParseWords is broken in this regards as it converts "\{\}\n" to
+    # "{}n" instead of keeping the backslashes here.
+    while ($cmdline =~ /
+        # Double quote
+        (")((?:[^\\"]+|\\.)*)"|
+        # Single quotes
+        (')([^']*)'|
+        # Whitespace
+        ([ \t]+)|
+        # Unquoted
+        ([^'"\\ \t]+|\\.)
+        /xsg) {
+        if (defined($1)) {      # Double quotes
+            # IEEE Std 1003.1 2004: only touch $ ` " \ <newline>
+            $arg .= $+ =~ s/\\([\$\`"\\\n])/$1/gr;
+        } elsif (defined($3)) { # Single quotes
+            $arg .= $+;
+        } elsif (defined($5)) { # Whitespace
+            push @args, $arg if defined($arg);
+            $arg = undef;
+        } else {                # Unquoted
+            $arg .= $+ =~ s/\\(.)/$1/gr;
+        }
+    }
+    push @args, $arg if defined($arg);
+    return @args;
+}
 
 #######################################################################
 # Memory allocation test and failure torture testing.
 #
 sub torture {
-    my $testcmd = shift;
-    my $gdbline = shift;
+    my %testcmd = %{shift()};
+    my %gdbline = %{shift()};
 
     # remove memdump first to be sure we get a new nice and clean one
     unlink($memdump);
 
     # First get URL from test server, ignore the output/result
-    runclient($testcmd);
+    runclient(%testcmd);
 
-    logmsg " CMD: $testcmd\n" if($verbose);
+    logmsg " CMD: " . cmd_tostring(%testcmd) . "\n" if($verbose);
 
     # memanalyze -v is our friend, get the number of allocations made
     my $count=0;
@@ -584,10 +651,10 @@ sub torture {
 
         my $ret = 0;
         if($gdbthis) {
-            runclient($gdbline)
+            runclient(%gdbline)
         }
         else {
-            $ret = runclient($testcmd);
+            $ret = runclient(%testcmd);
         }
         #logmsg "$_ Returned " . ($ret >> 8) . "\n";
 
@@ -723,20 +790,24 @@ sub verifyhttp {
         $bonus="1/";
     }
 
-    my $flags = "--max-time $server_response_maxtime ";
-    $flags .= "--output $verifyout ";
-    $flags .= "--silent ";
-    $flags .= "--verbose ";
-    $flags .= "--globoff ";
-    $flags .= "-1 "         if($has_axtls);
-    $flags .= "--insecure " if($proto eq 'https');
-    $flags .= "\"$proto://$ip:$port/${bonus}verifiedserver\"";
-
-    my $cmd = "$VCURL $flags 2>$verifylog";
+    my %cmd = (
+        cmd     => [
+            $VCURL,
+            "--max-time", $server_response_maxtime,
+            "--output", $verifyout,
+            "--silent",
+            "--verbose",
+            "--globoff",
+            $has_axtls ? "-1" : (),
+            $proto eq 'https' ?  "--insecure" : (),
+            "$proto://$ip:$port/${bonus}verifiedserver",
+        ],
+        stderr  => $verifylog,
+    );
 
     # verify if our/any server is running on this port
-    logmsg "RUN: $cmd\n" if($verbose);
-    my $res = runclient($cmd);
+    logmsg "RUN: " . cmd_tostring(%cmd) . "\n" if($verbose);
+    my $res = runclient(%cmd);
 
     $res >>= 8; # rotate the result
     if($res & 128) {
@@ -789,43 +860,41 @@ sub verifyftp {
     my $server = servername_id($proto, $ipvnum, $idnum);
     my $pid = 0;
     my $time=time();
-    my $extra="";
 
     my $verifylog = "$LOGDIR/".
         servername_canon($proto, $ipvnum, $idnum) .'_verify.log';
     unlink($verifylog) if(-f $verifylog);
 
-    if($proto eq "ftps") {
-        $extra .= "--insecure --ftp-ssl-control ";
-    }
-
-    my $flags = "--max-time $server_response_maxtime ";
-    $flags .= "--silent ";
-    $flags .= "--verbose ";
-    $flags .= "--globoff ";
-    $flags .= $extra;
-    $flags .= "\"$proto://$ip:$port/verifiedserver\"";
-
-    my $cmd = "$VCURL $flags 2>$verifylog";
+    my $data = '';
+    my %cmd = (
+        cmd     => [
+            $VCURL,
+            "--max-time", $server_response_maxtime,
+            "--silent",
+            "--verbose",
+            "--globoff",
+            $proto eq "ftps" ? ("--insecure", "--ftp-ssl-control") : (),
+            "$proto://$ip:$port/verifiedserver",
+        ],
+        stderr  => $verifylog,
+        stdout  => \$data,
+    );
 
     # check if this is our server running on this port:
-    logmsg "RUN: $cmd\n" if($verbose);
-    my @data = runclientoutput($cmd);
+    logmsg "RUN: " . cmd_tostring(%cmd) . "\n" if($verbose);
+    my $res = runclient(%cmd);
 
-    my $res = $? >> 8; # rotate the result
+    $res >>= 8; # rotate the result
     if($res & 128) {
         logmsg "RUN: curl command died with a coredump\n";
         return -1;
     }
 
-    foreach my $line (@data) {
-        if($line =~ /WE ROOLZ: (\d+)/) {
-            # this is our test server with a known pid!
-            $pid = 0+$1;
-            last;
-        }
+    if($data =~ /WE ROOLZ: (\d+)/) {
+        # this is our test server with a known pid!
+        $pid = 0+$1;
     }
-    if($pid <= 0 && @data && $data[0]) {
+    if($pid <= 0 && $data) {
         # this is not a known server
         logmsg "RUN: Unknown server on our $server port: $port\n";
         return 0;
@@ -861,19 +930,23 @@ sub verifyrtsp {
         servername_canon($proto, $ipvnum, $idnum) .'_verify.log';
     unlink($verifylog) if(-f $verifylog);
 
-    my $flags = "--max-time $server_response_maxtime ";
-    $flags .= "--output $verifyout ";
-    $flags .= "--silent ";
-    $flags .= "--verbose ";
-    $flags .= "--globoff ";
-    # currently verification is done using http
-    $flags .= "\"http://$ip:$port/verifiedserver\"";
-
-    my $cmd = "$VCURL $flags 2>$verifylog";
+    my %cmd = (
+        cmd     => [
+            $VCURL,
+            "--max-time", $server_response_maxtime,
+            "--output", $verifyout,
+            "--silent",
+            "--verbose",
+            "--globoff",
+            # currently verification is done using http
+            "http://$ip:$port/verifiedserver",
+        ],
+        stderr  => $verifylog,
+    );
 
     # verify if our/any server is running on this port
-    logmsg "RUN: $cmd\n" if($verbose);
-    my $res = runclient($cmd);
+    logmsg "RUN: " . cmd_tostring(%cmd) . "\n" if($verbose);
+    my $res = runclient(%cmd);
 
     $res >>= 8; # rotate the result
     if($res & 128) {
@@ -964,8 +1037,18 @@ sub verifysftp {
     }
     # Connect to sftp server, authenticate and run a remote pwd
     # command using our generated configuration and key files
-    my $cmd = "$sftp -b $sftpcmds -F $sftpconfig -S $ssh $ip > $sftplog 2>&1";
-    my $res = runclient($cmd);
+    my @cmd = (
+        $sftp,
+        "-b", $sftpcmds,
+        "-F", $sftpconfig,
+        "-S", $ssh,
+        $ip,
+    );
+    my $res = runclient(
+        cmd     => \@cmd,
+        stdout  => $sftplog,
+        stderr  => '&1',
+    );
     # Search for pwd command response in log file
     if(open(SFTPLOGFILE, "<$sftplog")) {
         while(<SFTPLOGFILE>) {
@@ -999,21 +1082,25 @@ sub verifyhttptls {
         servername_canon($proto, $ipvnum, $idnum) .'_verify.log';
     unlink($verifylog) if(-f $verifylog);
 
-    my $flags = "--max-time $server_response_maxtime ";
-    $flags .= "--output $verifyout ";
-    $flags .= "--verbose ";
-    $flags .= "--globoff ";
-    $flags .= "--insecure ";
-    $flags .= "--tlsauthtype SRP ";
-    $flags .= "--tlsuser jsmith ";
-    $flags .= "--tlspassword abc ";
-    $flags .= "\"https://$ip:$port/verifiedserver\"";
-
-    my $cmd = "$VCURL $flags 2>$verifylog";
+    my %cmd = (
+        cmd     => [
+            $VCURL,
+            "--max-time", $server_response_maxtime,
+            "--output", $verifyout,
+            "--verbose",
+            "--globoff",
+            "--insecure",
+            "--tlsauthtype", "SRP",
+            "--tlsuser", "jsmith",
+            "--tlspassword", "abc",
+            "https://$ip:$port/verifiedserver",
+        ],
+        stderr  => $verifylog,
+    );
 
     # verify if our/any server is running on this port
-    logmsg "RUN: $cmd\n" if($verbose);
-    my $res = runclient($cmd);
+    logmsg "RUN: " . cmd_tostring(%cmd) . "\n" if($verbose);
+    my $res = runclient(%cmd);
 
     $res >>= 8; # rotate the result
     if($res & 128) {
@@ -2253,12 +2340,19 @@ sub checksystem {
 
     my $curlverout="$LOGDIR/curlverout.log";
     my $curlvererr="$LOGDIR/curlvererr.log";
-    my $versioncmd="$CURL --version 1>$curlverout 2>$curlvererr";
+    my %versioncmd = (
+        cmd     => [
+            $CURL,
+            "--version",
+        ],
+        stdout  => $curlverout,
+        stderr  => $curlvererr,
+    );
 
     unlink($curlverout);
     unlink($curlvererr);
 
-    $versretval = runclient($versioncmd);
+    $versretval = runclient(%versioncmd);
     $versnoexec = $!;
 
     open(VERSOUT, "<$curlverout");
@@ -2439,7 +2533,7 @@ sub checksystem {
     if(!$curl) {
         logmsg "unable to get curl's version, further details are:\n";
         logmsg "issued command: \n";
-        logmsg "$versioncmd \n";
+        logmsg cmd_tostring(%versioncmd) . "\n";
         if ($versretval == -1) {
             logmsg "command failed with: \n";
             logmsg "$versnoexec \n";
@@ -2495,8 +2589,15 @@ sub checksystem {
     $has_shared = `sh $CURLCONFIG --built-shared`;
     chomp $has_shared;
 
-    my $hostname=join(' ', runclientoutput("hostname"));
-    my $hosttype=join(' ', runclientoutput("uname -a"));
+    my $hostname = my $hosttype = '';
+    runclient(
+        cmd     => ['hostname'],
+        stdout  => \$hostname,
+    );
+    runclient(
+        cmd     => ['uname', '-a'],
+        stdout  => \$hosttype,
+    );
 
     logmsg ("********* System characteristics ******** \n",
     "* $curl\n",
@@ -3088,7 +3189,7 @@ sub singletest {
             chomp $cmd;
             subVariables \$cmd;
             if($cmd) {
-                my @p = split(/ /, $cmd);
+                my @p = splitcmd("$cmd");
                 if($p[0] !~ /\//) {
                     # the first word, the command, does not contain a slash so
                     # we will scan the "improved" PATH to find the command to
@@ -3098,17 +3199,21 @@ sub singletest {
                     if($fullp) {
                         $p[0] = $fullp;
                     }
-                    $cmd = join(" ", @p);
                 }
 
-                my @o = `$cmd 2>/dev/null`;
-                if($o[0]) {
-                    $why = $o[0];
-                    chomp $why;
-                } elsif($?) {
+                my $out = "";
+                my %cmd = (
+                    cmd     => \@p,
+                    stderr  => File::Spec->devnull,
+                    stdout  => \$out,
+                );
+                my $rc = runclient(%cmd);
+                $why = (split(/\n/, $out))[0];
+                # Any output is assumed to be an error
+                if (!$why && $rc != 0) {
                     $why = "precheck command error";
                 }
-                logmsg "prechecked $cmd\n" if($verbose);
+                logmsg "prechecked " . cmd_tostring(%cmd) . "\n" if($verbose);
             }
         }
     }
@@ -3267,6 +3372,10 @@ sub singletest {
         }
     }
 
+    my %test_command = (
+        stdout  => $STDOUT,
+        stderr  => $STDERR,
+    );
     my $CMDLINE;
     my $cmdargs;
     my $cmdtype = $cmdhash{'type'} || "default";
@@ -3347,7 +3456,7 @@ sub singletest {
 
         writearray($stdinfile, \@stdintest);
 
-        $cmdargs .= " <$stdinfile";
+        $test_command{stdin} = $stdinfile;
     }
 
     if(!$tool) {
@@ -3370,13 +3479,17 @@ sub singletest {
         }
     }
 
-    $CMDLINE .= "$cmdargs >$STDOUT 2>$STDERR";
+    $CMDLINE .= "$cmdargs";
+
+    # TODO: get rid of $CMDLINE and consider using gdb --args ...
+    my @commandline_split = splitcmd("$CMDLINE");
+    $test_command{cmd} = \@commandline_split;
 
     if($verbose) {
-        logmsg "$CMDLINE\n";
+        logmsg cmd_tostring(%test_command) . "\n";
     }
 
-    print CMDLOG "$CMDLINE\n";
+    print CMDLOG cmd_tostring(%test_command) . "\n";
 
     unlink("core");
 
@@ -3412,16 +3525,31 @@ sub singletest {
 
     # run the command line we built
     if ($torture) {
-        $cmdres = torture($CMDLINE,
-                       "$gdb --directory libtest $DBGCURL -x $LOGDIR/gdbcmd");
+        my %gdb_command = (
+            cmd => [
+               $gdb,
+               "--directory", "libtest",
+               $DBGCURL,
+               "-x", "$LOGDIR/gdbcmd"
+            ],
+        );
+        $cmdres = torture(\%test_command, \%gdb_command);
     }
     elsif($gdbthis) {
         my $GDBW = ($gdbxwin) ? "-w" : "";
-        runclient("$gdb --directory libtest $DBGCURL $GDBW -x $LOGDIR/gdbcmd");
+        runclient(
+            cmd => [
+                $gdb,
+                "--directory", "libtest",
+                $DBGCURL,
+                $GDBW,
+                "-x", "$LOGDIR/gdbcmd",
+            ]
+        );
         $cmdres=0; # makes it always continue after a debugged run
     }
     else {
-        $cmdres = runclient("$CMDLINE");
+        $cmdres = runclient(%test_command);
         my $signal_num  = $cmdres & 127;
         $dumped_core = $cmdres & 128;
 
@@ -3451,7 +3579,16 @@ sub singletest {
             open(GDBCMD, ">$LOGDIR/gdbcmd2");
             print GDBCMD "bt\n";
             close(GDBCMD);
-            runclient("$gdb --directory libtest -x $LOGDIR/gdbcmd2 -batch $DBGCURL core ");
+            runclient(
+                cmd => [
+                    $gdb,
+                    "--directory", "libtest",
+                    "-x", "$LOGDIR/gdbcmd2",
+                    "-batch",
+                    $DBGCURL,
+                    "core",
+                ]
+            );
      #       unlink("$LOGDIR/gdbcmd2");
         }
     }
@@ -3564,7 +3701,10 @@ sub singletest {
         subVariables \$cmd;
         if($cmd) {
             logmsg "postcheck $cmd\n" if($verbose);
-            my $rc = runclient("$cmd");
+            my @cmda = splitcmd("$cmd");
+            my $rc = runclient(
+                cmd => \@cmda,
+            );
             # Must run the postcheck command in torture mode in order
             # to clean up, but the result can't be relied upon.
             if($rc != 0 && !$torture) {
@@ -4750,7 +4890,11 @@ if($valgrind) {
     # we have found valgrind on the host, use it
 
     # verify that we can invoke it fine
-    my $code = runclient("valgrind >/dev/null 2>&1");
+    my $code = runclient(
+        cmd     => ['valgrind'],
+        stdout  => File::Spec->devnull,
+        stderr  => '2>&1',
+    );
 
     if(($code>>8) != 1) {
         #logmsg "Valgrind failure, disable it\n";
@@ -4759,8 +4903,13 @@ if($valgrind) {
 
         # since valgrind 2.1.x, '--tool' option is mandatory
         # use it, if it is supported by the version installed on the system
-        runclient("valgrind --help 2>&1 | grep -- --tool > /dev/null 2>&1");
-        if (($? >> 8)==0) {
+        my $help_output = '';
+        runclient(
+            cmd     => ['valgrind', '--help'],
+            stdout  => \$help_output,
+            stderr  => '2>&1',
+        );
+        if ($help_output =~ /--tool/) {
             $valgrind_tool="--tool=memcheck";
         }
         open(C, "<$CURL");
@@ -4772,15 +4921,8 @@ if($valgrind) {
         close(C);
 
         # valgrind 3 renamed the --logfile option to --log-file!!!
-        my $ver=join(' ', runclientoutput("valgrind --version"));
-        # cut off all but digits and dots
-        $ver =~ s/[^0-9.]//g;
-
-        if($ver =~ /^(\d+)/) {
-            $ver = $1;
-            if($ver >= 3) {
-                $valgrind_logfile="--log-file";
-            }
+        if ($help_output =~ /--log-file/) {
+            $valgrind_logfile = "--log-file";
         }
     }
 }
-- 
2.1.2
--nextPart3719086.pcSmINMTvc
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLQpMaXN0IGFkbWluOiBodHRwOi8vY29vbC5oYXh4LnNlL2xpc3QvbGlzdGluZm8v
Y3VybC1saWJyYXJ5CkV0aXF1ZXR0ZTogIGh0dHA6Ly9jdXJsLmhheHguc2UvbWFpbC9ldGlxdWV0
dGUuaHRtbA==
--nextPart3719086.pcSmINMTvc--
Received on 2001-09-17