retitle 782672 [PATCH] sbuild: Extra command parsing is overly-complex and
broken
tags 782672 patch
thanks
I made a patch to address this. It's attached. The patch says that it's
2/3 in a series, but this is the only one relevant to this issue.
Notes:
when handling external commands (--starting-build-commands and others),
I now keep the input string as is, and pass it to the shell, instead of
splitting it on whitespace and then reassembling it. This reassembling
was being done improperly, with shell internals such as < being
erroneously escaped (doing it right is way more work than simply keeping
the original string). Keeping the string as is allows the user to give
the command they want.
This is implemented by keeping the COMMAND and INTCOMMAND keys same as
they were (array-refs of strings), but adds additional COMMAND_STR and
INTCOMMAND_STR keys that have the string representation. The non-STR
versions are always populated, and the external command options populate
the _STR ones as well. When running the commands, we use the _STR
options if we have them, otherwise, we recombine as before.
Since the old array-ref-based code path is still there, old commandlines
and configurations remain working.
The big win so far is that I can get an interactive shell going. It's
not ideal yet since I have to manually redirect the pty, and there's
some stdout buffering thing I haven't tried to fix, but it's already
super useful:
$ ls -l /dev/stdin(:A)
crw------- 1 dkogan tty 136, 1 Apr 21 12:42 /dev/pts/1
$ sbuild .... --starting-build-commands 'sh -i < /dev/pts/1 >
/dev/pts/1'
+------------------------------------------------------------------------------+
| Starting Timed Build Commands
|
+------------------------------------------------------------------------------+
sh -i < /dev/pts/1 > /dev/pts/1
-------------------------------
pwd
/build/cross-gcc-4.9-i386-uMV_ms
>From 9eeea9238147bce12e5544751b6e4106df24571c Mon Sep 17 00:00:00 2001
From: Dima Kogan <d...@secretsauce.net>
Date: Mon, 20 Apr 2015 22:12:56 -0700
Subject: [PATCH 2/3] external commands are now unparsed strings
when handling external commands (--starting-build-commands and others), I now
keep the input string as is, and pass it to the shell, instead of splitting it
on whitespace and then reassembling it. This reassembling was being done
improperly, with shell internals such as < being erroneously escaped. Keeping
the string as is allows the user to give the command they want.
This is implemented by keeping the COMMAND and INTCOMMAND keys same as they were
(array-refs of strings), but adds additional COMMAND_STR and INTCOMMAND_STR keys
that have the string representation. The non-STR versions are always populated,
and the external command options populate the _STR ones as well. When running
the commands, we use the _STR options if we have them, otherwise, we recombine
as before.
Since the old array-ref-based code path is still there, old commandlines and
configurations remain working
---
lib/Sbuild/Build.pm | 71 +++++++++++++++++++++++++++++++--------------
lib/Sbuild/Chroot.pm | 4 +++
lib/Sbuild/ChrootPlain.pm | 41 +++++++++++++++-----------
lib/Sbuild/ChrootSchroot.pm | 14 ++++++---
lib/Sbuild/ChrootSudo.pm | 43 +++++++++++++++------------
lib/Sbuild/Conf.pm | 2 +-
lib/Sbuild/Options.pm | 21 +++++---------
7 files changed, 119 insertions(+), 77 deletions(-)
diff --git a/lib/Sbuild/Build.pm b/lib/Sbuild/Build.pm
index b44ccb3..03e5522 100644
--- a/lib/Sbuild/Build.pm
+++ b/lib/Sbuild/Build.pm
@@ -1081,23 +1081,37 @@ sub run_command {
$defaults = $self->get('Host')->{'Defaults'};
$out = $defaults->{'STREAMOUT'} if ($log_output);
$err = $defaults->{'STREAMERR'} if ($log_error);
- $self->get('Host')->run_command(
- { COMMAND => \@{$command},
- PRIORITY => 0,
- STREAMOUT => $out,
- STREAMERR => $err,
- });
+
+ my %args = (PRIORITY => 0,
+ STREAMOUT => $out,
+ STREAMERR => $err);
+ if(ref $command) {
+ $args{COMMAND} = \@{$command};
+ $args{COMMAND_STR} = "@{$command}";
+ } else {
+ $args{COMMAND} = [split('\s+', $command)];
+ $args{COMMAND_STR} = $command;
+ }
+
+ $self->get('Host')->run_command( \%args );
} else {
$defaults = $self->get('Session')->{'Defaults'};
$out = $defaults->{'STREAMOUT'} if ($log_output);
$err = $defaults->{'STREAMERR'} if ($log_error);
- $self->get('Session')->run_command(
- { COMMAND => \@{$command},
- USER => ($rootuser ? 'root' : $self->get_conf('BUILD_USER')),
- PRIORITY => 0,
- STREAMOUT => $out,
- STREAMERR => $err,
- });
+
+ my %args = (USER => ($rootuser ? 'root' : $self->get_conf('BUILD_USER')),
+ PRIORITY => 0,
+ STREAMOUT => $out,
+ STREAMERR => $err);
+ if(ref $command) {
+ $args{COMMAND} = \@{$command};
+ $args{COMMAND_STR} = "@{$command}";
+ } else {
+ $args{COMMAND} = [split('\s+', $command)];
+ $args{COMMAND_STR} = $command;
+ }
+
+ $self->get('Session')->run_command( \%args );
}
my $status = $?;
@@ -1172,17 +1186,30 @@ sub run_external_commands {
sort {length $b <=> length $a || $a cmp $b} keys %percent);
my $returnval = 1;
foreach my $command (@commands) {
- foreach my $arg (@{$command}) {
- $arg =~ s{
- # Match a percent followed by a valid keyword
- \%($keyword_pat)
- }{
- # Substitute with the appropriate value only if it's defined
- $percent{$1} || $&
- }msxge;
+
+ my $substitute = sub {
+ foreach(@_) {
+ s{
+ # Match a percent followed by a valid keyword
+ \%($keyword_pat)
+ }{
+ # Substitute with the appropriate value only if it's defined
+ $percent{$1} || $&
+ }msxge;
+ }
+ };
+
+ my $command_str;
+ if( ref $command ) {
+ $substitute->(@{$command});
+ $command_str = join(" ", @{$command});
+ } else {
+ $substitute->($command);
+ $command_str = $command;
}
- my $command_str = join(" ", @{$command});
+
$self->log_subsubsection("$command_str");
+
$returnval = $self->run_command($command, $log_output, $log_error, $chroot, $rootuser);
$self->log("\n");
if (!$returnval) {
diff --git a/lib/Sbuild/Chroot.pm b/lib/Sbuild/Chroot.pm
index ad193c8..366fb1e 100644
--- a/lib/Sbuild/Chroot.pm
+++ b/lib/Sbuild/Chroot.pm
@@ -242,6 +242,7 @@ sub run_command {
my $options = shift;
$options->{'INTCOMMAND'} = copy($options->{'COMMAND'});
+ $options->{'INTCOMMAND_STR'} = copy($options->{'COMMAND_STR'});
return $self->run_command_internal($options);
}
@@ -254,6 +255,7 @@ sub pipe_command {
my $options = shift;
$options->{'INTCOMMAND'} = copy($options->{'COMMAND'});
+ $options->{'INTCOMMAND_STR'} = copy($options->{'COMMAND_STR'});
return $self->pipe_command_internal($options);
}
@@ -306,7 +308,9 @@ sub exec_command {
debug2("PROGRAM: $program\n");
debug2("COMMAND: ", join(" ", @{$options->{'COMMAND'}}), "\n");
+ debug2("COMMAND_STR: ", $options->{'COMMAND'} // 'UNDEFINED', "\n");
debug2("INTCOMMAND: ", join(" ", @{$options->{'INTCOMMAND'}}), "\n");
+ debug2("INTCOMMAND_STR: ", $options->{'INTCOMMAND_STR:'} // 'UNDEFINED', "\n");
debug2("EXPCOMMAND: ", join(" ", @{$options->{'EXPCOMMAND'}}), "\n");
debug2("Environment set:\n");
diff --git a/lib/Sbuild/ChrootPlain.pm b/lib/Sbuild/ChrootPlain.pm
index 477bb7a..e8c8ee6 100644
--- a/lib/Sbuild/ChrootPlain.pm
+++ b/lib/Sbuild/ChrootPlain.pm
@@ -80,7 +80,9 @@ sub get_command_internal {
my $self = shift;
my $options = shift;
- my $command = $options->{'INTCOMMAND'}; # Command to run
+ # Command to run. If I have a string, use it. Otherwise use the list-ref
+ my $command = $options->{'INTCOMMAND_STR'} // $options->{'INTCOMMAND'};
+
my $user = $options->{'USER'}; # User to run command under
my $dir; # Directory to use (optional)
$dir = $self->get('Defaults')->{'DIR'} if
@@ -99,20 +101,6 @@ sub get_command_internal {
$dir = '/';
}
- my $shellcommand;
- foreach (@$command) {
- my $tmp = $_;
- $tmp =~ s/'//g; # Strip any single quotes for security
- if ($_ ne $tmp) {
- $self->log_warning("Stripped single quote from command for security: $_\n");
- }
- if ($shellcommand) {
- $shellcommand .= " '$tmp'";
- } else {
- $shellcommand = "'$tmp'";
- }
- }
-
my $need_chroot = 0;
$need_chroot = 1
if ($self->get('Location') ne '/');
@@ -128,10 +116,29 @@ sub get_command_internal {
if ($need_chroot);
push(@cmdline, $self->get_conf('SU'), "$user", '-s')
if ($need_su);
- push(@cmdline, '/bin/sh', '-c', "cd '$dir' && $shellcommand");
+
+
+ if( ref $command ) {
+ my $shellcommand;
+ foreach (@$command) {
+ my $tmp = $_;
+ $tmp =~ s/'//g; # Strip any single quotes for security
+ if ($_ ne $tmp) {
+ $self->log_warning("Stripped single quote from command for security: $_\n");
+ }
+ if ($shellcommand) {
+ $shellcommand .= " '$tmp'";
+ } else {
+ $shellcommand = "'$tmp'";
+ }
+ }
+ push(@cmdline, '/bin/sh', '-c', "cd '$dir' && $shellcommand");
+ } else {
+ push(@cmdline, '/bin/sh', '-c', "cd '$dir' && ( $command )");
+ }
$options->{'USER'} = $user;
- $options->{'COMMAND'} = $command;
+ $options->{'COMMAND'} = ref($command) ? $command : [split(/\s+/, $command)];
$options->{'EXPCOMMAND'} = \@cmdline;
$options->{'CHDIR'} = undef;
$options->{'DIR'} = $dir;
diff --git a/lib/Sbuild/ChrootSchroot.pm b/lib/Sbuild/ChrootSchroot.pm
index 3a4ab13..b11cabc 100644
--- a/lib/Sbuild/ChrootSchroot.pm
+++ b/lib/Sbuild/ChrootSchroot.pm
@@ -102,7 +102,9 @@ sub get_command_internal {
my $self = shift;
my $options = shift;
- my $command = $options->{'INTCOMMAND'}; # Command to run
+ # Command to run. If I have a string, use it. Otherwise use the list-ref
+ my $command = $options->{'INTCOMMAND_STR'} // $options->{'INTCOMMAND'};
+
my $user = $options->{'USER'}; # User to run command under
my $dir; # Directory to use (optional)
$dir = $self->get('Defaults')->{'DIR'} if
@@ -125,9 +127,13 @@ sub get_command_internal {
'-c', $self->get('Session ID'),
'--run-session',
@{$self->get_conf('SCHROOT_OPTIONS')},
- '-u', "$user", '-p', '--',
- @$command);
-
+ '-u', "$user", '-p', '--');
+ if (ref $command) {
+ push @cmdline, @$command;
+ } else {
+ push @cmdline, ('/bin/sh', '-c', $command);
+ $command = [split(/\s+/, $command)];
+ }
$options->{'USER'} = $user;
$options->{'COMMAND'} = $command;
$options->{'EXPCOMMAND'} = \@cmdline;
diff --git a/lib/Sbuild/ChrootSudo.pm b/lib/Sbuild/ChrootSudo.pm
index b7cc623..b36642c 100644
--- a/lib/Sbuild/ChrootSudo.pm
+++ b/lib/Sbuild/ChrootSudo.pm
@@ -83,7 +83,9 @@ sub get_command_internal {
my $self = shift;
my $options = shift;
- my $command = $options->{'INTCOMMAND'}; # Command to run
+ # Command to run. If I have a string, use it. Otherwise use the list-ref
+ my $command = $options->{'INTCOMMAND_STR'} // $options->{'INTCOMMAND'};
+
my $user = $options->{'USER'}; # User to run command under
my $dir; # Directory to use (optional)
$dir = $self->get('Defaults')->{'DIR'} if
@@ -102,27 +104,30 @@ sub get_command_internal {
$dir = '/';
}
- my $shellcommand;
- foreach (@$command) {
- my $tmp = $_;
- $tmp =~ s/'//g; # Strip any single quotes for security
- if ($_ ne $tmp) {
- $self->log_warning("Stripped single quote from command for security: $_\n");
- }
- if ($shellcommand) {
- $shellcommand .= " '$tmp'";
- } else {
- $shellcommand = "'$tmp'";
- }
- }
-
@cmdline = ($self->get_conf('SUDO'), '/usr/sbin/chroot', $self->get('Location'),
- $self->get_conf('SU'), "$user", '-s',
- '/bin/sh', '-c',
- "cd '$dir' && $shellcommand");
+ $self->get_conf('SU'), "$user", '-s');
+
+ if( ref $command ) {
+ my $shellcommand;
+ foreach (@$command) {
+ my $tmp = $_;
+ $tmp =~ s/'//g; # Strip any single quotes for security
+ if ($_ ne $tmp) {
+ $self->log_warning("Stripped single quote from command for security: $_\n");
+ }
+ if ($shellcommand) {
+ $shellcommand .= " '$tmp'";
+ } else {
+ $shellcommand = "'$tmp'";
+ }
+ }
+ push(@cmdline, '/bin/sh', '-c', "cd '$dir' && $shellcommand");
+ } else {
+ push(@cmdline, '/bin/sh', '-c', "cd '$dir' && ( $command )");
+ }
$options->{'USER'} = $user;
- $options->{'COMMAND'} = $command;
+ $options->{'COMMAND'} = ref($command) ? $command : [split(/\s+/, $command)];
$options->{'EXPCOMMAND'} = \@cmdline;
$options->{'CHDIR'} = undef;
$options->{'DIR'} = $dir;
diff --git a/lib/Sbuild/Conf.pm b/lib/Sbuild/Conf.pm
index 6ccbba5..d8e5f0c 100644
--- a/lib/Sbuild/Conf.pm
+++ b/lib/Sbuild/Conf.pm
@@ -965,7 +965,7 @@ sub setup ($) {
HELP => 'Preceding arguments to launch piuparts as root. If no arguments are specified, piuparts will be launched via sudo.'
},
'EXTERNAL_COMMANDS' => {
- TYPE => 'HASH:ARRAY:ARRAY:STRING',
+ TYPE => 'HASH:ARRAY:STRING',
VARNAME => 'external_commands',
GROUP => 'Chroot options',
DEFAULT => {
diff --git a/lib/Sbuild/Options.pm b/lib/Sbuild/Options.pm
index d978f71..10e02f2 100644
--- a/lib/Sbuild/Options.pm
+++ b/lib/Sbuild/Options.pm
@@ -204,9 +204,8 @@ sub set_options {
$self->set_conf('STATS_DIR', $_[1]);
},
"setup-hook=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"chroot-setup-commands"}},
- \@command);
+ $_[1]);
$self->set_conf('CHROOT_SETUP_SCRIPT', $_[1]);
},
"use-snapshot" => sub {
@@ -268,34 +267,28 @@ sub set_options {
$_[1]);
},
"pre-build-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"pre-build-commands"}},
- \@command);
+ $_[1]);
},
"chroot-setup-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"chroot-setup-commands"}},
- \@command);
+ $_[1]);
},
"starting-build-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"starting-build-commands"}},
- \@command);
+ $_[1]);
},
"finished-build-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"finished-build-commands"}},
- \@command);
+ $_[1]);
},
"chroot-cleanup-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"chroot-cleanup-commands"}},
- \@command);
+ $_[1]);
},
"post-build-commands=s" => sub {
- my @command = split(/\s+/, $_[1]);
push(@{${$self->get_conf('EXTERNAL_COMMANDS')}{"post-build-commands"}},
- \@command);
+ $_[1]);
},
"log-external-command-output" => sub {
$self->set_conf('LOG_EXTERNAL_COMMAND_OUTPUT', 1);
--
2.1.4
