in reply to upload.cgi my script is bugged

There are a number of changes/recommendation's I'd make here.

  1. Always, always, always run under use strict and use warnings.
  2. Use the CGI module's function for printing the HTML header, rather than writing it yourself.
  3. Define all your files & file paths right up front, rather than having them scattered through the file. I haven't done it here, but you might even consider making them constants.
  4. Simplify your regexp, and make sure it does what you mean; anchor it to the end of the string, so it won't match my_file.jpeg.txt.
  5. Use a better system for tracking the file number. The subroutine I've used here is taken from the Perl Cookbook, and uses file locking to prevent two programs (or the same program) from writing to the file at the same time. This is an important consideration anytime you're doing file I/O from the web.
  6. The CGI.pm docs recommend using $cgi->upload for upload fields.

So, finally, here's my code:

#!/usr/bin/perl use strict ; use warnings ; use CGI ; use CGI::Carp qw( fatalsToBrowser ) ; use Fcntl qw( :DEFAULT :flock ) ; my $cgi = new CGI ; print $cgi->header ; my $dir = "/www/test" ; my $count = "/www/test/counter.dat" ; my $file = $cgi->param( 'file' ) ; if ( $file =~ /.(jpe?g|gif)$/i ) { accept_file( $file, $dir, next_file_number( $count ), $1 ) ; } else { print <<"END_OF_MSG" ; <h1>$file</h1> Sorry, but you have either not selected a file to upload or you are trying to upload an invalid picture file. Only the following file extensions are supported: /.jpeg/.gif/.jpg. Please press the 'back' button below to return to the Upload form. END_OF_MSG } sub accept_file { my ( $file, $path, $file_num, $ext ) = @_ ; open FH, ">$path/$file_num.$ext" or die "Can't open $path/$file_num.$ext: $!" ; binmode FH ; while ( <$file> ) { print FH $_; } close FH ; } sub next_file_number { my $counter_file = shift ; sysopen FH, $counter_file, O_RDWR|O_CREAT or die "Can't open $counter_file: $!" ; flock( FH, LOCK_EX ) or die "Can't write-lock $counter_file: $! +" ; # Now we have acquired the lock, it's safe for I/O my $num = <FH> || 0 ; seek FH, 0, 0 or die "Can't rewind $counter_file: $!" ; truncate FH, 0 or die "Can't truncate $counter_file: $!" +; print FH ++$num, "\n" or die "Can't write $counter_file: $!" ; close FH or die "Can't close $counter_file: $!" ; return $num ; }

_______________
DamnDirtyApe
Those who know that they are profound strive for clarity. Those who
would like to seem profound to the crowd strive for obscurity.
            --Friedrich Nietzsche

Replies are listed 'Best First'.
Re: Re: upload.cgi my script is bugged
by Anonymous Monk on Aug 12, 2002 at 01:04 UTC
    Thank's for your reply, will give it a go Pip