in reply to Code review, good 'ol CGI.

A few points. When locking a file I use this little snippet to allow a script to wait for its lock:

my $flock = 1; # set to true to flock my $timeout = 10; # set to number of seconds before flock timeout .... if ($flock) { my $count = 0; until (flock FILE, LOCK_SH) { sleep 1; DieNice("Can't lock file '$file': $!\n") if ++$count >= $timeout +; } }

Also closing a filehandle removes the underlying file lock so you only need to close them, Perl will unlock them for you thus all the LOCK_UN lines are redundant.

This line does nothing useful, it does not untaint $mode

$mode =~ tr#a-z_##cd; # untaint that param!

This bit uses $mode in a regex and the value in $1 assigned to $page is untainted.

if ( $mode =~ /^([a-z_]+)_go$/ ) { $page = $1 or die "no page given!";

The or die makes no logical sense as the assignment $page = $1 will always succeed thus this can never execute.

You have a number of or die "blah" statements. You should include the $! special var as this stores Perl's explanation of the error that has triggered the die ie "File does not exist". The standard synatax goes like:

open FILE, ">>$file" or die "Can't append to $file, Perl says $!\n";

This code is a very obtuse way to do something quite simple, namely go to one of two subs depending on the user input.

sub main { my %dispatch = ( upload => \&upload, debug => \&debug, DEFAULT => \&upload, ); $draw->headers(); $draw->html_start( { title => $title } ); my $action = $cgi->param('action') || 'DEFAULT'; my $do_sub = $dispatch{ $action }; if ( $do_sub ) { &$do_sub(); } else { warn "[warn] Invalid action: [$action]"; die "[die] You can't make me do that!\n"; } $draw->html_end(); }

You create a hash of sub references and then eventually call them. This is much easier to follow:

sub main { $draw->headers(); $draw->html_start( { title => $title } ); if ($cgi->param('action') eq 'debug'){ &debug; } else { &upload; } $draw->html_end(); }

The logic is the same with the exception of not checking for 'incorrect' action params. This is the typical logic I use as it is simple - either you ask for something with the correct name or you get the default page. The use of warn and die as you do is also redundant. Both warn and die print to STDERR. Warn just does this whereas die throws an expception afterwards. Logically you are just doing a die here.

Your open files in two different places in the script then lock them all in the one place. This makes no logical sense to me and makes it hard to follow your logic train.

Finally in CGI die is oftem suboptimal. You should not run a production script with use CGI::Carp qw( fatalsToBrowser ) active as it makes it easier to hack a script as the exact errors are reported in the browser. Without carp die will give the user a 500 error when it gets called. For these reasons amongst others most developers write a die_nice routine. Here is a die_nice routine:

sub DieNice { my $message = shift; my ($package, $file, $line) = caller(); my $user_message = "Sorry, the system is currently unable to proce +ss your request<br>\n"; $user_message .= "due to routine maintenance. Please try again lat +er. Our Apologies\n"; $message = Unindent <<" MESSAGE"; A fatal die was trapped by the $scriptname die nice routine, detai +ls: Time: $datetime List name: $list_name Script name: $scriptname Package: $package File: $file Line Number: $line Error Message: $message MESSAGE &TellUser($user_message); # the bullshit message! &WarnAdmin($message); # the real facts! exit; } sub WarnAdmin { my $message = shift; my $name = "Administrator"; my $email = $our_email; &EmailUser ($name, $email, $message); return; } sub Unindent { my $unindent = shift; $unindent =~ s/^[ \t]+//gm; return $unindent; }

The unindent sub just lets me indent the herepage stuff with the sub and drop this indentation off the final output. Hope this helps, all up looks good with -Tw, use strict, setting a secure path etc.