Michael Vilain said:
Wow, what a bad idea, in 90% of cases, including yours.
Here's the code from that book:
In the main script, call the function:
&parse_form_data(*FORM);
Wow, I can think of at least three things wrong with that function
call already-- can you?
which will put your variables into the %FORM hash.
and here's the routine:
sub parse_form_data
{
local (*FORM_DATA) = @_;
Why would you do this? eeeevil. If you must do such a thing, declare
the hash %FORM_DATA with 'my', and return it at the end of this
function. Better yet, don't, and use CGI.pm.
local ( $request_method, $query_string, @key_value_pairs,
$key_value, $key, $value, $arg_value, $arg_index );
All bad! Use lexical variables here instead. It's easy enough to
do-- swap 'my' for 'local' there. You haven't had to use 'local' for
private variables since perl 5 came out ages and ages ago. It's also
bad style in general to use package variables when they aren't needed.
$request_method = $ENV{'REQUEST_METHOD'};
if ( $request_method eq "" && $ENV{'SHELL'} ne "" ) {
$request_method = "SHELL";
}
Note: CGI.pm handles all this for you. And better than this code
does.
if ($request_method eq "GET") {
$query_string = $ENV{'QUERY_STRING'};
} elsif ($request_method eq "POST") {
read (STDIN, $query_string, $ENV{'CONTENT_LENGTH'});
You don't check that you read $ENV{CONTENT_LENGTH} (no quotes needed
there, or anywhere you have used a hash here. That could be bad, but
you'll never know!
} elsif ($request_method eq "SHELL") {
#
# for the shell, build a string of arguments so it can be
# parsed later on
#
$arg_index = 1;
foreach $arg_value (@ARGV) {
$arg_value =~ s/=/%3D/g;
$value = sprintf ("ARG%d=%s",$arg_index,$arg_value);
if ($arg_index == 1) {
$query_string = $value;
} else {
$query_string = join ("&", $query_string, $value);
}
$arg_index++;
}
You don't do proper URL escaping on your keys or values there. CGI.pm
handles this properly.
} else {
&return_error (500, "Server Error",
"Server uses unsupported method");
Why are you calling functions with &? That hasn't been necessary for
years now.
}
@key_value_pairs = split (/&/, $query_string);
What if your user agent submits parameters separated by ';' instead of
'&'? This is perfectly legal to do, and you don't handle it at all.
foreach $key_value (@key_value_pairs) {
($key, $value) = split (/=/, $key_value);
$value =~ tr/+/ /;
$value =~ s/%([\dA-Fa-f][\dA-Fa-f])/pack ("C", hex ($1))/eg;
I haven't checked, but I would bet this isn't a complete URL-decoding
algorithm.
$value =~ s/[;><&\*`\|\!]//g; # prevent sub-shell nasties
What sub shell? I see no sub shell here....
if (defined($FORM_DATA{$key})) {
$FORM_DATA{$key} = join ("\0", $FORM_DATA{$key}, $value);
Ick. How annoying; it makes you manually detect multiple values
yourself and parse them out again manually. CGI.pm will happily hand
you an list if you get multiple values for one named parameter.
} else {
$FORM_DATA{$key} = $value;
}
}
}
I stopped using this when CGI.pm came out and haven't looked back since.
Good advice! I was afraid you were still using this rather buggy
code. I would advise anyone else still using this code to stop using
it as well, and use CGI.pm.
And to forestall Gunnar, yes, it is possible to write your own correct
URL-parsing code, if you know what you're doing. This isn't it.
That's why, unless you have a good reason not to, you should use
CGI.pm.
-=Eric
RIVATE_TEMPFILES = 1;