Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/lib/WineTestBot/Utils.pm | 100 +++++++++++++++++-------------- 1 file changed, 54 insertions(+), 46 deletions(-)
diff --git a/testbot/lib/WineTestBot/Utils.pm b/testbot/lib/WineTestBot/Utils.pm index 81a2eda39..8b0bfeb0c 100644 --- a/testbot/lib/WineTestBot/Utils.pm +++ b/testbot/lib/WineTestBot/Utils.pm @@ -35,6 +35,10 @@ use Fcntl; use WineTestBot::Config;
+# +# Web helpers +# + sub MakeSecureURL($) { my ($URL) = @_; @@ -53,6 +57,56 @@ sub SecureConnection() return defined($ENV{"HTTPS"}) && $ENV{"HTTPS"} eq "on"; }
+sub DurationToString($;$) +{ + my ($Secs, $Raw) = @_; + + return "n/a" if (!defined $Secs); + + my @Parts; + if (!$Raw) + { + my $Mins = int($Secs / 60); + $Secs -= 60 * $Mins; + my $Hours = int($Mins / 60); + $Mins -= 60 * $Hours; + my $Days = int($Hours / 24); + $Hours -= 24 * $Days; + push @Parts, "${Days}d" if ($Days); + push @Parts, "${Hours}h" if ($Hours); + push @Parts, "${Mins}m" if ($Mins); + } + if (!@Parts or int($Secs) != 0) + { + push @Parts, (@Parts or int($Secs) == $Secs) ? + int($Secs) ."s" : + sprintf('%.1fs', $Secs); + } + return join(" ", @Parts); +} + +sub BuildEMailRecipient($$) +{ + my ($EMailAddress, $Name) = @_; + + if (! defined($EMailAddress)) + { + return undef; + } + my $Recipient = "<" . $EMailAddress . ">"; + if ($Name) + { + $Recipient .= " ($Name)"; + } + + return $Recipient; +} + + +# +# Temporary file helpers +# + sub GenerateRandomString($) { my ($Len) = @_; @@ -119,50 +173,4 @@ sub CreateNewDir($$) } }
-sub DurationToString($;$) -{ - my ($Secs, $Raw) = @_; - - return "n/a" if (!defined $Secs); - - my @Parts; - if (!$Raw) - { - my $Mins = int($Secs / 60); - $Secs -= 60 * $Mins; - my $Hours = int($Mins / 60); - $Mins -= 60 * $Hours; - my $Days = int($Hours / 24); - $Hours -= 24 * $Days; - push @Parts, "${Days}d" if ($Days); - push @Parts, "${Hours}h" if ($Hours); - push @Parts, "${Mins}m" if ($Mins); - } - if (!@Parts or int($Secs) != 0) - { - push @Parts, (@Parts or int($Secs) == $Secs) ? - int($Secs) ."s" : - sprintf('%.1fs', $Secs); - } - return join(" ", @Parts); -} - -sub BuildEMailRecipient($$) -{ - my ($EMailAddress, $Name) = @_; - - if (! defined($EMailAddress)) - { - return undef; - } - my $Recipient = "<" . $EMailAddress . ">"; - if ($Name) - { - $Recipient .= " ($Name)"; - } - - return $Recipient; -} - - 1;
IsValidFileName() verifies that the filename is valid on both Windows and Unix. This is necessary to ensure we will be able to upload the file to the build and/or test VMs. IsValidFileName() is defined in the Utils.pm module so it can be reused where necessary.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/lib/WineTestBot/Utils.pm | 25 ++++++++++++++++++++++++- testbot/web/Submit.pl | 4 ++-- 2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/testbot/lib/WineTestBot/Utils.pm b/testbot/lib/WineTestBot/Utils.pm index 8b0bfeb0c..962e6ff67 100644 --- a/testbot/lib/WineTestBot/Utils.pm +++ b/testbot/lib/WineTestBot/Utils.pm @@ -28,7 +28,7 @@ WineTestBot::Utils - Utility functions use Exporter 'import'; our @EXPORT = qw(MakeSecureURL SecureConnection GenerateRandomString OpenNewFile CreateNewFile CreateNewLink CreateNewDir - DurationToString BuildEMailRecipient); + DurationToString BuildEMailRecipient IsValidFileName);
use Fcntl;
@@ -173,4 +173,27 @@ sub CreateNewDir($$) } }
+ +# +# Shell helpers +# + +=pod +=over 12 + +=item C<IsValidFileName()> + +Returns true if the filename is valid on Unix and Windows systems. + +This also ensures this is not a trick filename such as '../important/file'. + +=back +=cut + +sub IsValidFileName($) +{ + my ($FileName) = @_; + return $FileName !~ m~[<>:"/\|?*]~; +} + 1; diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl index c16b99f1e..afebc722b 100644 --- a/testbot/web/Submit.pl +++ b/testbot/web/Submit.pl @@ -514,10 +514,10 @@ sub ValidateAndGetFileName($$) $self->{ErrMessage} = "You must provide a file to test"; return undef; } - if ($FileName =~ m=[/\]=) + if (!IsValidFileName($FileName)) { $self->{ErrField} = $FieldName; - $self->{ErrMessage} = "The filename is invalid"; + $self->{ErrMessage} = "The filename contains invalid characters"; return undef; } my $PropertyDescriptor = CreateSteps()->GetPropertyDescriptorByName("FileName");
Tainting is moot because the whole purpose of the script is to run arbitrary user-provided code (in patch form). Still validate the patch filename but use the standard IsValidFileName() function (instead of the much stricter regular expression used so far) so patches accepted by Submit.pl don't get rejected at this late stage. However carefully quote the filename when building our shell command so it does not fail if the filename contains a space (for instance). Also note that in practice (so far) the patch filename is always called patch.diff as this is hardcoded in WineRunBuild.pl. So this is all a bit academic.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/bin/build/Build.pl | 25 ++++++++++++++----------- testbot/lib/WineTestBot/Utils.pm | 26 +++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/testbot/bin/build/Build.pl b/testbot/bin/build/Build.pl index 8318af12a..e97995559 100755 --- a/testbot/bin/build/Build.pl +++ b/testbot/bin/build/Build.pl @@ -1,10 +1,13 @@ -#!/usr/bin/perl -Tw +#!/usr/bin/perl # -*- Mode: Perl; perl-indent-level: 2; indent-tabs-mode: nil -*- # # Performs the 'build' task in the build machine. Specifically this applies a # conformance test patch, rebuilds the impacted test and retrieves the # resulting 32 and 64 bit binaries. # +# This script does not use tainting (-T) because its whole purpose is to run +# arbitrary user-provided code anyway (in patch form). +# # Copyright 2009 Ge van Geldorp # Copyright 2012-2014, 2017-2018 Francois Gouget # @@ -22,6 +25,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+use warnings; use strict;
sub BEGIN @@ -42,6 +46,8 @@ sub BEGIN
use WineTestBot::Config; use WineTestBot::PatchUtils; +use WineTestBot::Utils; +
my $LogFileName = "$LogDir/Build.log";
@@ -85,7 +91,7 @@ sub ApplyPatch($)
InfoMsg "Applying patch\n"; system("( cd $DataDir/wine && set -x && " . - " git apply --verbose $PatchFile && " . + " git apply --verbose ". ShQuote($PatchFile) ." && ". " git add -A " . ") >>$LogFileName 2>&1"); if ($? != 0) @@ -199,18 +205,15 @@ if (! $PatchFile || !$BitIndicators) FatalError "Usage: Build.pl <patchfile> <bits>\n"; }
-# Untaint parameters -if ($PatchFile =~ m/^([\w_.-]+)$/) +# Verify parameters +if (!IsValidFileName($PatchFile)) { - $PatchFile = "$DataDir/staging/$1"; - if (! -r $PatchFile) - { - FatalError "Patch file $PatchFile not readable\n"; - } + FatalError "The patch filename '$PatchFile' contains invalid characters\n"; } -else +$PatchFile = "$DataDir/staging/$PatchFile"; +if (!-r $PatchFile) { - FatalError "Invalid patch file $PatchFile\n"; + FatalError "Patch file '$PatchFile' is not readable\n"; }
my ($Run32, $Run64); diff --git a/testbot/lib/WineTestBot/Utils.pm b/testbot/lib/WineTestBot/Utils.pm index 962e6ff67..111a56589 100644 --- a/testbot/lib/WineTestBot/Utils.pm +++ b/testbot/lib/WineTestBot/Utils.pm @@ -28,7 +28,8 @@ WineTestBot::Utils - Utility functions use Exporter 'import'; our @EXPORT = qw(MakeSecureURL SecureConnection GenerateRandomString OpenNewFile CreateNewFile CreateNewLink CreateNewDir - DurationToString BuildEMailRecipient IsValidFileName); + DurationToString BuildEMailRecipient IsValidFileName + ShQuote);
use Fcntl;
@@ -196,4 +197,27 @@ sub IsValidFileName($) return $FileName !~ m~[<>:"/\|?*]~; }
+=pod +=over 12 + +=item C<ShQuote()> + +Quotes strings so they can be used in shell commands. + +Note that this implies escaping '$'s and '`'s which may not be appropriate +in another context. + +=back +=cut + +sub ShQuote($) +{ + my ($Str)=@_; + $Str =~ s%\%\\%g; + $Str =~ s%$%\$%g; + $Str =~ s%"%\"%g; + $Str =~ s%`%\`%g; + return ""$Str""; +} + 1;
The Engine-side scripts (WineRun*.pl) already redirect stdout and stderr to the appropriate log. So it does not make sense to delete that log and then add new redirects for every command started from the client scripts. Remove FatalError() since it's not used in Reconfig.pl.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/bin/build/Build.pl | 42 ++++++++------------------ testbot/bin/build/Reconfig.pl | 55 +++++++++++------------------------ 2 files changed, 29 insertions(+), 68 deletions(-)
diff --git a/testbot/bin/build/Build.pl b/testbot/bin/build/Build.pl index e97995559..c23bb8780 100755 --- a/testbot/bin/build/Build.pl +++ b/testbot/bin/build/Build.pl @@ -49,27 +49,19 @@ use WineTestBot::PatchUtils; use WineTestBot::Utils;
-my $LogFileName = "$LogDir/Build.log"; - sub InfoMsg(@) { - my $OldUMask = umask(002); - if (open(my $Log, ">>", $LogFileName)) - { - print $Log @_; - close($Log); - } - umask($OldUMask); + print @_; }
sub LogMsg(@) { - InfoMsg "Build: ", @_; + print "Build: ", @_; }
sub FatalError(@) { - LogMsg @_; + print STDERR @_; exit 1; }
@@ -90,10 +82,9 @@ sub ApplyPatch($) my ($PatchFile) = @_;
InfoMsg "Applying patch\n"; - system("( cd $DataDir/wine && set -x && " . - " git apply --verbose ". ShQuote($PatchFile) ." && ". - " git add -A " . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/wine && set -x && ". + "git apply --verbose ". ShQuote($PatchFile) ." && ". + "git add -A"); if ($? != 0) { LogMsg "Patch failed to apply\n"; @@ -104,9 +95,7 @@ sub ApplyPatch($) if ($Impacts->{Makefiles}) { InfoMsg "\nRunning make_makefiles\n"; - system("( cd $DataDir/wine && set -x && " . - " ./tools/make_makefiles " . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/wine && set -x && ./tools/make_makefiles"); if ($? != 0) { LogMsg "make_makefiles failed\n"; @@ -117,9 +106,7 @@ sub ApplyPatch($) if ($Impacts->{Autoconf} && !$Impacts->{HasConfigure}) { InfoMsg "\nRunning autoconf\n"; - system("( cd $DataDir/wine && set -x && " . - " autoconf " . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/wine && set -x && autoconf"); if ($? != 0) { LogMsg "Autoconf failed\n"; @@ -135,9 +122,8 @@ sub BuildNative() mkdir "$DataDir/build-native" if (! -d "$DataDir/build-native");
InfoMsg "\nRebuilding native tools\n"; - system("( cd $DataDir/build-native && set -x && " . - " time make -j$ncpus __tooldeps__ " . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/build-native && set -x && ". + "time make -j$ncpus __tooldeps__"); if ($? != 0) { LogMsg "Rebuild of native tools failed\n"; @@ -161,9 +147,8 @@ sub BuildTestExecutables($$) }
InfoMsg "\nBuilding the $Bits-bit test executable(s)\n"; - system("( cd $DataDir/build-mingw$Bits && set -x && " . - " time make -j$ncpus ". join(" ", sort @BuildDirs) . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/build-mingw$Bits && set -x && ". + "time make -j$ncpus ". join(" ", sort @BuildDirs)); if ($? != 0) { LogMsg "Rebuild of $Bits-bit crossbuild failed\n"; @@ -186,9 +171,6 @@ sub BuildTestExecutables($$) $ENV{PATH} = "/usr/lib/ccache:/usr/bin:/bin"; delete $ENV{ENV};
-# Start with a clean logfile -unlink($LogFileName); - my ($PatchFile, $BitIndicators); if (@ARGV == 2) { diff --git a/testbot/bin/build/Reconfig.pl b/testbot/bin/build/Reconfig.pl index c950d4974..2da2f54a4 100755 --- a/testbot/bin/build/Reconfig.pl +++ b/testbot/bin/build/Reconfig.pl @@ -43,28 +43,15 @@ sub BEGIN use WineTestBot::Config; use WineTestBot::PatchUtils;
-my $LogFileName = "$LogDir/Reconfig.log";
sub InfoMsg(@) { - my $OldUMask = umask(002); - if (open(my $Log, ">>", $LogFileName)) - { - print $Log @_; - close($Log); - } - umask($OldUMask); + print @_; }
sub LogMsg(@) { - InfoMsg "Reconfig: ", @_; -} - -sub FatalError(@) -{ - LogMsg @_; - exit 1; + print "Reconfig: ", @_; }
my $ncpus; @@ -86,9 +73,8 @@ sub BuildTestAgentd() if (! -x "$BinDir/build/testagentd") { InfoMsg "\nBuilding the native testagentd\n"; - system("( cd $::RootDir/src/testagentd && set -x && " . - " time make -j$ncpus build " . - ") >>$LogFileName 2>&1"); + system("cd $::RootDir/src/testagentd && set -x && ". + "time make -j$ncpus build"); if ($? != 0) { LogMsg "Build testagentd failed\n"; @@ -97,9 +83,8 @@ sub BuildTestAgentd() }
InfoMsg "\nRebuilding the Windows TestAgentd\n"; - system("( cd $::RootDir/src/testagentd && set -x && " . - " time make -j$ncpus iso " . - ") >>$LogFileName 2>&1"); + system("cd $::RootDir/src/testagentd && set -x && ". + "time make -j$ncpus iso"); if ($? != 0) { LogMsg "Build winetestbot.iso failed\n"; @@ -112,9 +97,8 @@ sub BuildTestAgentd() sub BuildTestLauncher() { InfoMsg "\nRebuilding TestLauncher\n"; - system("( cd $::RootDir/src/TestLauncher && set -x && " . - " time make -j$ncpus" . - ") >>$LogFileName 2>&1"); + system("cd $::RootDir/src/TestLauncher && set -x && ". + "time make -j$ncpus"); if ($? != 0) { LogMsg "Build TestLauncher failed\n"; @@ -127,7 +111,7 @@ sub BuildTestLauncher() sub GitPull() { InfoMsg "\nUpdating the Wine source\n"; - system("cd $DataDir/wine && git pull >>$LogFileName 2>&1"); + system("cd $DataDir/wine && git pull"); if ($? != 0) { LogMsg "Git pull failed\n"; @@ -150,11 +134,10 @@ sub BuildNative()
# Rebuild from scratch to make sure cruft will not accumulate InfoMsg "\nRebuilding native tools\n"; - system("( cd $DataDir/build-native && set -x && " . - " rm -rf * && " . - " time ../wine/configure --enable-win64 --without-x --without-freetype --disable-winetest && " . - " time make -j$ncpus __tooldeps__ " . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/build-native && set -x && ". + "rm -rf * && ". + "time ../wine/configure --enable-win64 --without-x --without-freetype --disable-winetest && ". + "time make -j$ncpus __tooldeps__");
if ($? != 0) { @@ -174,11 +157,10 @@ sub BuildCross($)
# Rebuild from scratch to make sure cruft will not accumulate InfoMsg "\nRebuilding the $Bits-bit test executables\n"; - system("( cd $DataDir/build-mingw$Bits && set -x && " . - " rm -rf * && " . - " time ../wine/configure --host=$Host --with-wine-tools=../build-native --without-x --without-freetype --disable-winetest && " . - " time make -j$ncpus buildtests" . - ") >>$LogFileName 2>&1"); + system("cd $DataDir/build-mingw$Bits && set -x && ". + "rm -rf * && ". + "time ../wine/configure --host=$Host --with-wine-tools=../build-native --without-x --without-freetype --disable-winetest && ". + "time make -j$ncpus buildtests"); if ($? != 0) { LogMsg "Build cross ($Bits bits) failed\n"; @@ -191,9 +173,6 @@ sub BuildCross($) $ENV{PATH} = "/usr/lib/ccache:/usr/bin:/bin"; delete $ENV{ENV};
-# Start with a clean logfile -unlink($LogFileName); - if (! -d "$DataDir/staging" and ! mkdir "$DataDir/staging") { LogMsg "Unable to create '$DataDir/staging': $!\n";
-
Francois Gouget