{"thread":{"id":"37997","subject":"[PATCH] gitweb: hack around CGI's list-context param() handling","startedAt":"2014-11-18T17:10:22Z","lastAt":"2014-11-18T18:01:35Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252088","messageId":"20141118171022.GA18799@peff.net","threadId":"37997","inReplyTo":null,"subject":"[PATCH] gitweb: hack around CGI's list-context param() handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-18T17:10:22Z","receivedAt":"2014-11-18T17:10:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As of CGI.pm's 4.08 release, the behavior to call\nCGI::param() in a list context is deprecated (because it can\nbe potentially unsafe if called inside a hash constructor).\nThis cause gitweb to issue a warning for some of our code,\nwhich in turn causes the tests to fail.\n\nOur use is in fact _not_ one of the dangerous cases, as we\nare intentionally using a list context. The recommended\nroute by 4.08 is to use the new CGI::multi_param() call to\nmake it explicit that we know what we are doing.\nHowever, that function is only available in 4.08, which is\nabout a month old; we cannot rely on having it.\n\nOne option would be to set $CGI::LIST_CONTEXT_WARN globally,\nwhich turns off the warning. However, that would eliminate\nthe protection these newer releases are trying to provide.\nWe want to annotate each site as OK using the new function.\n\nSo instead, let's check whether CGI provides the\nmulti_param() function, and if not, provide an\nimplementation that just wraps param(). That will work on\nboth old and new versions of CGI. Sadly, we cannot just\ncheck defined(\\&CGI::multi_param), because CGI uses the\nautoload feature, which claims that all functions are\ndefined. Instead, we just do a version check.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWithout this patch, all of the gitweb tests consistently fail on Debian\ntesting/unstable when you have libcgi-pm-perl installed (it works\nwithout that package installed, because an older version of CGI.pm is in\nthe perl base). I tested with both versions.\n\nAnother approach would be to live with the warning, but teach the tests\nto be less meticulous. I think it probably makes sense to keep them\npedantic, though, because it can catch potential errors.\n\n gitweb/gitweb.perl | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ccf7516..7a5b23a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -20,6 +20,10 @@ use File::Basename qw(basename);\n use Time::HiRes qw(gettimeofday tv_interval);\n binmode STDOUT, ':utf8';\n \n+if (!defined($CGI::VERSION) || $CGI::VERSION < 4.08) {\n+\teval 'sub CGI::multi_param { CGI::param(@_) }'\n+}\n+\n our $t0 = [ gettimeofday() ];\n our $number_of_git_cmds = 0;\n \n@@ -871,7 +875,7 @@ sub evaluate_query_params {\n \n \twhile (my ($name, $symbol) = each %cgi_param_mapping) {\n \t\tif ($symbol eq 'opt') {\n-\t\t\t$input_params{$name} = [ map { decode_utf8($_) } $cgi->param($symbol) ];\n+\t\t\t$input_params{$name} = [ map { decode_utf8($_) } $cgi->multi_param($symbol) ];\n \t\t} else {\n \t\t\t$input_params{$name} = decode_utf8($cgi->param($symbol));\n \t\t}\n-- \n2.1.2.596.g7379948\n"},{"id":"252095","messageId":"xmqqfvdgwful.fsf@gitster.dls.corp.google.com","threadId":"37997","inReplyTo":"20141118171022.GA18799@peff.net","subject":"Re: [PATCH] gitweb: hack around CGI's list-context param() handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T17:58:26Z","receivedAt":"2014-11-18T17:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> As of CGI.pm's 4.08 release, the behavior to call\n> CGI::param() in a list context is deprecated (because it can\n> be potentially unsafe if called inside a hash constructor).\n> This cause gitweb to issue a warning for some of our code,\n> which in turn causes the tests to fail.\n>\n> Our use is in fact _not_ one of the dangerous cases, as we\n> are intentionally using a list context. The recommended\n> route by 4.08 is to use the new CGI::multi_param() call to\n> make it explicit that we know what we are doing.\n> However, that function is only available in 4.08, which is\n> about a month old; we cannot rely on having it.\n>\n> One option would be to set $CGI::LIST_CONTEXT_WARN globally,\n> which turns off the warning. However, that would eliminate\n> the protection these newer releases are trying to provide.\n> We want to annotate each site as OK using the new function.\n>\n> So instead, let's check whether CGI provides the\n> multi_param() function, and if not, provide an\n> implementation that just wraps param(). That will work on\n> both old and new versions of CGI. Sadly, we cannot just\n> check defined(\\&CGI::multi_param), because CGI uses the\n> autoload feature, which claims that all functions are\n> defined. Instead, we just do a version check.\n\nThe solution does look like the best in the messy situation.  Thanks\nfor looking into it.\n\n\n> +if (!defined($CGI::VERSION) || $CGI::VERSION < 4.08) {\n> +\teval 'sub CGI::multi_param { CGI::param(@_) }'\n> +}\n> +\n>  our $t0 = [ gettimeofday() ];\n>  our $number_of_git_cmds = 0;\n>  \n> @@ -871,7 +875,7 @@ sub evaluate_query_params {\n>  \n>  \twhile (my ($name, $symbol) = each %cgi_param_mapping) {\n>  \t\tif ($symbol eq 'opt') {\n> -\t\t\t$input_params{$name} = [ map { decode_utf8($_) } $cgi->param($symbol) ];\n> +\t\t\t$input_params{$name} = [ map { decode_utf8($_) } $cgi->multi_param($symbol) ];\n>  \t\t} else {\n>  \t\t\t$input_params{$name} = decode_utf8($cgi->param($symbol));\n>  \t\t}\n"},{"id":"252096","messageId":"20141118180135.GA12461@peff.net","threadId":"37997","inReplyTo":"xmqqfvdgwful.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] gitweb: hack around CGI's list-context param() handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-18T18:01:35Z","receivedAt":"2014-11-18T18:01:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 09:58:26AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > As of CGI.pm's 4.08 release, the behavior to call\n> > CGI::param() in a list context is deprecated (because it can\n> > be potentially unsafe if called inside a hash constructor).\n> > This cause gitweb to issue a warning for some of our code,\n\nOops, s/cause/causes/ of course.\n\n-Peff\n"}]}