[buildd-tools-devel] [PATCH] Improve sbuild's use of the perl system() function.
Paul Wise
pabs at debian.org
Mon May 2 09:39:13 UTC 2016
Replace with perl-native functions where possible.
Avoid shell meta-character injection where possible.
Avoid extra forks due to shell where possible.
Avoid manually running shell where possible.
Avoid option injection where possible.
---
bin/sbuild-adduser | 2 +-
bin/sbuild-createchroot | 7 ++++---
bin/sbuild-debuild | 2 +-
lib/Buildd/Daemon.pm | 9 ++++-----
lib/Buildd/Mail.pm | 8 ++++----
lib/Buildd/Watcher.pm | 16 ++++++++--------
lib/Sbuild.pm | 2 +-
lib/Sbuild/Build.pm | 2 +-
lib/Sbuild/ChrootSchroot.pm | 2 +-
lib/Sbuild/ChrootSetup.pm | 2 +-
10 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/bin/sbuild-adduser b/bin/sbuild-adduser
index a0fe3d0..033a4ed 100755
--- a/bin/sbuild-adduser
+++ b/bin/sbuild-adduser
@@ -41,7 +41,7 @@ foreach (@ARGV) {
my $user = getpwnam($_);
if (defined $user) {
- $status += system("/usr/sbin/adduser", "$_", "sbuild");
+ $status += system(qw(/usr/sbin/adduser --), $_, 'sbuild');
} else {
print STDERR "W: User \"$_\" does not exist\n";
$status++;
diff --git a/bin/sbuild-createchroot b/bin/sbuild-createchroot
index 64b193d..6216371 100755
--- a/bin/sbuild-createchroot
+++ b/bin/sbuild-createchroot
@@ -256,9 +256,10 @@ EOF
close POLICY_RC_D or die "Can't close $policy_rc_d";
-!system("chown", "root:", "$policy_rc_d")
+my (undef, undef, $uid, undef) = getpwnam('root');
+!chown($uid, -1, $policy_rc_d)
or die "E: Failed to set root: ownership on $policy_rc_d";
-!system("chmod", "0775", "$policy_rc_d")
+!chmod(0775, $policy_rc_d)
or die "E: Failed to set 0755 permissions on $policy_rc_d";
# Display /usr/sbin/policy-rc.d.
@@ -310,7 +311,7 @@ EOF
# Determine whether system has overlayfs capability
my $uniontype = "union-type=none";
if (lc("$^O") =~ /linux/) {
- system("/sbin/modprobe overlay");
+ system(qw(/sbin/modprobe overlay));
if (open(FILE, "/proc/filesystems")) {
if (grep {/\soverlay$/} <FILE>) {
$uniontype = "union-type=overlay";
diff --git a/bin/sbuild-debuild b/bin/sbuild-debuild
index 59de804..ca15a77 100644
--- a/bin/sbuild-debuild
+++ b/bin/sbuild-debuild
@@ -362,7 +362,7 @@ sub process_package {
if ((!$conf->get('NO_LINTIAN')) && (-x '/usr/bin/lintian')) {
print "Running lintian.\n";
- system('/usr/bin/lintian', @{$conf->get('LINTIAN_OPTS')}, $bin_changes);
+ system('/usr/bin/lintian', @{$conf->get('LINTIAN_OPTS')}, '--', $bin_changes);
if (($? >> 8) != 0) {
print "Running lintian failed: $?";
chdir($currentdir);
diff --git a/lib/Buildd/Daemon.pm b/lib/Buildd/Daemon.pm
index 9c75537..0f876c3 100644
--- a/lib/Buildd/Daemon.pm
+++ b/lib/Buildd/Daemon.pm
@@ -701,11 +701,10 @@ sub move_to_upload {
unlink( $upload_path );
$self->log("'$upload_path' removed.\n");
}
- system sprintf('dcmd mv %s/build/%s %s/%s/',
- $self->get_conf('HOME'),
- $changes_name,
- $self->get_conf('HOME'),
- $dist_config->get('DUPLOAD_LOCAL_QUEUE_DIR'));
+ system(qw(dcmd mv --),
+ sprintf('%s/build/%s', $self->get_conf('HOME'), $changes_name),
+ sprintf(%s/%s/', $self->get_conf('HOME'), $dist_config->get('DUPLOAD_LOCAL_QUEUE_DIR'))
+ );
$self->log("$pv moved to '$upload_dir'\n");
}
diff --git a/lib/Buildd/Mail.pm b/lib/Buildd/Mail.pm
index 4e0acd7..1180ec2 100644
--- a/lib/Buildd/Mail.pm
+++ b/lib/Buildd/Mail.pm
@@ -381,9 +381,9 @@ FILE: foreach (@to_remove) {
# that only has build-depends in it.
# if that's too much cpu, have buildd use perl-apt if avail to export the
# build-depends list, which could then be read in at this point
- if (system "mv $upload_dir/$_ " .
+ if (system (qw(mv --), "$upload_dir/$_",
$self->get_conf('HOME') .
- "/build/chroot-$dist/var/cache/apt/archives/") {
+ "/build/chroot-$dist/var/cache/apt/archives/")) {
$self->log("Cannot move $upload_dir/$_ to cache dir\n");
} else {
next FILE;
@@ -591,7 +591,7 @@ sub prepare_for_upload ($$) {
for my $upload_dir (@upload_dirs) {
lock_file( $upload_dir );
foreach (@files) {
- if (system "cp " . $self->get_conf('HOME') . "/build/$_ $upload_dir/$_") {
+ if (system('cp', '--', $self->get_conf('HOME')."/build/$_", "$upload_dir/$_")) {
$self->log("Cannot copy $_ to $upload_dir/\n");
++$errs;
}
@@ -605,7 +605,7 @@ sub prepare_for_upload ($$) {
}
foreach (@files) {
- if (system "rm " . $self->get_conf('HOME') . "/build/$_") {
+ if (!unlink($self->get_conf('HOME') . "/build/$_")) {
$self->log("Cannot remove build/$_\n");
++$errs;
}
diff --git a/lib/Buildd/Watcher.pm b/lib/Buildd/Watcher.pm
index 4d9d3e0..91b8b29 100644
--- a/lib/Buildd/Watcher.pm
+++ b/lib/Buildd/Watcher.pm
@@ -121,7 +121,7 @@ sub run {
foreach (@to_purge) {
next if ! -d $_;
- system "sudo rm -rf $_";
+ system(qw(sudo rm -rf --), $_);
$self->log("Purged $_\n");
}
@@ -227,18 +227,18 @@ sub run {
-M "old-logs/daemon-stamp" > $self->get_conf('DAEMON_LOG_ROTATE')-$self->get('Fudge')) {
$self->log("Rotating daemon log file\n");
- system "touch old-logs/daemon-stamp";
+ system(qw(touch old-logs/daemon-stamp));
my $d = $self->format_time(time);
if (-f $self->get_conf('DAEMON_LOG_FILE') . ".old") {
- system "mv " . $self->get_conf('DAEMON_LOG_FILE') . ".old old-logs/daemon-$d.log";
- system "gzip -9 old-logs/daemon-$d.log";
+ system(qw(mv --), $self->get_conf('DAEMON_LOG_FILE') . '.old', "old-logs/daemon-$d.log";
+ system(qw(gzip -9), "old-logs/daemon-$d.log";
}
rename( $self->get_conf('DAEMON_LOG_FILE'),
$self->get_conf('DAEMON_LOG_FILE') . ".old" );
my $old_umask = umask 0007;
- system "touch " . $self->get_conf('DAEMON_LOG_FILE');
+ system(qw(touch --), $self->get_conf('DAEMON_LOG_FILE'));
umask $old_umask;
kill( 1, $daemon_pid ) if $daemon_pid;
$self->reopen_log();
@@ -306,7 +306,7 @@ sub archive_logs ($$$$) {
return if -f "$destpat-stamp" && -M "$destpat-stamp" < $minage-$self->get('Fudge');
$self->log("Archiving logs in $dir:\n");
- system "touch $destpat-stamp";
+ system(qw(touch --), "$destpat-stamp");
$olddir = cwd;
chdir( $dir );
@@ -326,8 +326,8 @@ sub archive_logs ($$$$) {
$newt = $self->format_time($newest);
$file = $self->get_conf('HOME') . "/$destpat-$oldt-$newt.tar";
- system "tar cf $file @todo";
- system "gzip -9 $file";
+ system(qw(tar cf), $file, '--', @todo);
+ system(qw(gzip -9 --), $file);
if ($dir eq "logs") {
local (*F);
diff --git a/lib/Sbuild.pm b/lib/Sbuild.pm
index 0971752..f11f639 100644
--- a/lib/Sbuild.pm
+++ b/lib/Sbuild.pm
@@ -249,7 +249,7 @@ sub help_text ($$) {
my $section = shift;
my $page = shift;
- system("/usr/bin/man", "$section", "$page");
+ system(qw('man --', $section, $page);
exit 0;
}
diff --git a/lib/Sbuild/Build.pm b/lib/Sbuild/Build.pm
index a9596f5..c4ec64c 100644
--- a/lib/Sbuild/Build.pm
+++ b/lib/Sbuild/Build.pm
@@ -2624,7 +2624,7 @@ sub close_build_log {
if (!defined($changes)) {
$self->log_error(".changes is undef. Cannot sign .changes.\n");
} else {
- system "debsign", "-k$key_id", "$build_dir/$changes";
+ system('debsign', "-k$key_id", '--', "$build_dir/$changes";
}
}
}
diff --git a/lib/Sbuild/ChrootSchroot.pm b/lib/Sbuild/ChrootSchroot.pm
index 743bb8e..d712062 100644
--- a/lib/Sbuild/ChrootSchroot.pm
+++ b/lib/Sbuild/ChrootSchroot.pm
@@ -89,7 +89,7 @@ sub end_session {
print STDERR "Cleaning up chroot (session id " . $self->get('Session ID') . ")\n"
if $self->get_conf('DEBUG');
- system($self->get_conf('SCHROOT') . ' -c ' . $self->get('Session ID') . ' --end-session');
+ system($self->get_conf('SCHROOT'), '-c', $self->get('Session ID'), '--end-session');
$self->set('Session ID', "");
if ($?) {
print STDERR "Chroot cleanup failed\n";
diff --git a/lib/Sbuild/ChrootSetup.pm b/lib/Sbuild/ChrootSetup.pm
index 3e409b2..a6a5e92 100644
--- a/lib/Sbuild/ChrootSetup.pm
+++ b/lib/Sbuild/ChrootSetup.pm
@@ -59,7 +59,7 @@ sub basesetup ($$) {
# This will require root privileges. However, this should
# only get run at initial chroot setup time.
my $groupfile = $session->get('Location') . "/etc/group";
- system '/bin/sh', '-c', "getent group sbuild >> $groupfile";
+ system("getent group sbuild >> $groupfile");
if ($?) {
print STDERR "E: Failed to create group sbuild\n";
return $?
--
2.8.1
More information about the Buildd-tools-devel
mailing list