Wrapper: allocate new environment dynamically

Otherwise, if third-party plugins extend newenviron by more than
3 entries, we could overflow the array. It seems unlikely that any
third-party plugin manipulates newenviron in practice, so this
is mostly theoretical. Just in case, I have deliberately avoided
using "i" as the variable name, so that any third-party plugin
that was manipulating newenviron directly will now result in the
wrapper failing to compile.

I have not assumed that realloc(NULL, ...) works as an equivalent of
malloc(...), in case there are still operating systems where that
doesn't work.

diff --git a/IkiWiki/Receive.pm b/IkiWiki/Receive.pm
index 5908e09f953ad09cb576cae6c956230be497d6df..332ba7c2c378d45123c915115c243379940e9bbf 100644 (file)
--- a/IkiWiki/Receive.pm
+++ b/IkiWiki/Receive.pm
@@ -46,8 +46,8 @@ EOF
                        while (read(0, &buf, 256) != 0) {}
                        exit(0);
                }
-               asprintf(&s, "CALLER_UID=%i", u);
-               newenviron[i++]=s;
+               asprintf(&s, "%i", u);
+               addenv("CALLER_UID", s);
        }
 EOF
        return $ret;

diff --git a/IkiWiki/Wrapper.pm b/IkiWiki/Wrapper.pm
index 69500029cd20ae104bac6160dc8f1c18cb5dfa3d..a8de39eea13b7d6ac83be34e4c7735b9ca9f5478 100644 (file)
--- a/IkiWiki/Wrapper.pm
+++ b/IkiWiki/Wrapper.pm
@@ -52,7 +52,6 @@ sub gen_wrapper () {
                       HTTP_COOKIE REMOTE_USER HTTPS REDIRECT_STATUS
                       HTTP_HOST SERVER_PORT HTTPS HTTP_ACCEPT
                       REDIRECT_URL} if $config{cgi};
-       my $envsize=$#envsave;
        my $envsave="";
        foreach my $var (@envsave) {
                $envsave.=<<"EOF";
@@ -65,7 +64,6 @@ EOF
                        my $val=$config{ENV}{$key};
                        utf8::encode($val) if utf8::is_utf8($val);
                        $val =~ s/([^A-Za-z0-9])/sprintf '""\\x%02x""', ord($1)/ge;
-                       $envsize += 1;
                        $envsave.=<<"EOF";
        addenv("$key", "$val");
 EOF
@@ -184,18 +182,33 @@ EOF
 #include <sys/file.h>
 
 extern char **environ;
-char *newenviron[$envsize+7];
-int i=0;
+int newenvironlen=0;
+/* Array of length newenvironlen+1 (+1 for NULL) */
+char **newenviron=NULL;
 
 void addenv(char *var, char *val) {
-       char *s=malloc(strlen(var)+1+strlen(val)+1);
+       char *s;
+
+       if (newenviron) {
+               newenviron=realloc(newenviron, (newenvironlen+2) * sizeof(char *));
+       }
+       else {
+               newenviron=calloc(newenvironlen+2, sizeof(char *));
+       }
+
+       if (!newenviron) {
+               perror("realloc");
+               exit(1);
+       }
+
+       s=malloc(strlen(var)+1+strlen(val)+1);
        if (!s) {
                perror("malloc");
                exit(1);
        }
        else {
                sprintf(s, "%s=%s", var, val);
-               newenviron[i++]=s;
+               newenviron[newenvironlen++]=s;
        }
 }
 
@@ -215,9 +228,9 @@ int main (int argc, char **argv) {
 $check_commit_hook
 @wrapper_hooks
 $envsave
-       newenviron[i++]="HOME=$ENV{HOME}";
-       newenviron[i++]="PATH=$ENV{PATH}";
-       newenviron[i++]="WRAPPED_OPTIONS=$configstring";
+       addenv("HOME", "$ENV{HOME}");
+       addenv("PATH", "$ENV{PATH}");
+       addenv("WRAPPED_OPTIONS", "$configstring");
 
 #ifdef __TINYC__
        /* old tcc versions do not support modifying environ directly */
@@ -225,10 +238,10 @@ $envsave
                perror("clearenv");
                exit(1);
        }
-       for (; i>0; i--)
-               putenv(newenviron[i-1]);
+       for (; newenvironlen>0; newenvironlen--)
+               putenv(newenviron[newenvironlen-1]);
 #else
-       newenviron[i]=NULL;
+       newenviron[newenvironlen]=NULL;
        environ=newenviron;
 #endif
 

diff --git a/debian/changelog b/debian/changelog
index 81a67928286d046a57efc3943d00a10e0f4f6f23..e721f167f3b549bbfea5569446e06de0492c9120 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+ikiwiki (3.20160510) UNRELEASED; urgency=medium
+
+  * wrappers: allocate new environment dynamically, so we won't overrun
+    the array if third-party plugins add multiple environment variables.
+
+ -- Simon McVittie <smcv@debian.org>  Wed, 11 May 2016 09:15:51 +0100
+
 ikiwiki (3.20160509) unstable; urgency=high
 
   [ Amitai Schlair ]

diff --git a/t/cvs.t b/t/cvs.t
index 371c21ec9f7ab53418ac344f5545e6347a11046d..6acafd701fe39f29ea3732983784439c929eefad 100755 (executable)
--- a/t/cvs.t
+++ b/t/cvs.t
@@ -708,5 +708,5 @@ sub stripext {
 }
 
 sub _wrapper_paths {
-       return qq{newenviron[i++]="PERL5LIB=$ENV{PERL5LIB}";};
+       return qq{addenv("PERL5LIB", "$ENV{PERL5LIB}");};
 }

diff --git a/t/wrapper-environ.t b/t/wrapper-environ.t
new file mode 100755 (executable)
index 0000000..ddf5a6e
--- /dev/null
+++ b/t/wrapper-environ.t
@@ -0,0 +1,92 @@
+#!/usr/bin/perl
+use warnings;
+use strict;
+
+use Test::More;
+plan(skip_all => "IPC::Run not available")
+       unless eval q{
+               use IPC::Run qw(run);
+               1;
+       };
+
+use IkiWiki;
+
+use Cwd qw(getcwd);
+use Errno qw(ENOENT);
+
+my $installed = $ENV{INSTALLED_TESTS};
+
+my @command;
+if ($installed) {
+       @command = qw(env PERL5LIB=t/tmp ikiwiki);
+}
+else {
+       ok(! system("make -s ikiwiki.out"));
+       @command = qw(env PERL5LIB=t/tmp:blib/lib:blib/arch perl -I. ./ikiwiki.out
+               --underlaydir=underlays/basewiki
+               --set underlaydirbase=underlays
+               --templatedir=templates);
+}
+
+writefile("test.setup", "t/tmp", <<EOF
+# IkiWiki::Setup::Yaml - YAML formatted setup file
+wikiname: this is the name of my wiki
+srcdir: t/tmp/in
+destdir: t/tmp/out
+url: http://localhost
+cgiurl: http://localhost/ikiwiki.cgi
+cgi_wrapper: t/tmp/ikiwiki.cgi
+cgi_wrappermode: 0754
+add_plugins:
+- anonok
+- excessiveenvironment
+anonok_pagespec: "*"
+ENV: { 'PERL5LIB': 't/tmp:blib/lib:blib/arch' }
+EOF
+       );
+
+writefile("index.mdwn", "t/tmp/in", "");
+
+writefile("IkiWiki/Plugin/excessiveenvironment.pm", "t/tmp", <<'EOF'
+#!/usr/bin/perl
+package IkiWiki::Plugin::excessiveenvironment;
+use warnings;
+use strict;
+use IkiWiki;
+
+sub import {
+       hook(type => "getsetup", id => "excessiveenvironment", call => \&getsetup);
+       hook(type => "genwrapper", id => "excessiveenvironment", call => \&genwrapper);
+}
+
+sub getsetup {
+       return plugin => {
+               safe => 0,
+               rebuild => undef,
+               section => "rcs",
+       };
+}
+
+sub genwrapper {
+       my @ret;
+       foreach my $j (1..4096) {
+               push @ret, qq{addenv("VAR$j", "val$j");\n};
+       }
+       return join '', @ret;
+}
+
+1;
+EOF
+       );
+
+my $stdout;
+ok(! system(@command, qw(--setup t/tmp/test.setup --rebuild --wrappers)), "run ikiwiki");
+ok(run(["./t/tmp/ikiwiki.cgi"], '<&-', '>', \$stdout, init => sub {
+       $ENV{HTTP_HOST} = "localhost";
+       $ENV{QUERY_STRING} = "do=prefs";
+       $ENV{REQUEST_METHOD} = "GET";
+       $ENV{SCRIPT_NAME} = "/cgi-bin/ikiwiki.cgi";
+       $ENV{SERVER_PORT} = "80"
+}), "run CGI");
+
+done_testing();