CGI.pm file upload freaking me out

[Trimbach]

I'm going over some older code of mine with the intention of resolving some intermittent problems users have had with the file upload feature in one of my CGI's. I have a CGI that allows users to upload a picture of themselves (a la Perl Monks)... very simple. The file size limit is 30K, and I've limited it to JPEGS only.

Anyway, my original code used the "param is a filename and a filehandle" technique from CGI.pm like:

use CGI qw(:standard);

$photo_save_name ="something";
    
my $file_name = param('photo');
   
if ($file_name !~ /\.jpe?g$/) {
    $error = "You can only upload a picture in JPEG format.";
}
    
open (F, ">$photo_save_name");

while (<$file_name>) {
    print F;
}

There's lots of problems here: 1) it doesn't work under strict, 2) checking MIME is better than checking regex, 3) It doesn't check for filesize. I checked out Ovid's nice (Ovid) Re: File Upload To Selected Directory to get going but none of the file upload features from CGI.pm seem to be working.

For example:

• $CGI::POST_MAX = 36000; is not preventing uploads of (for example) 100k+ files
• my $fh = upload($file_name); is not returning a filehandle... and it doesn't throw an error, even though $file_name is verified to exist (as a string, at least)
• my $tmpfile=tmpFileName($file_name); returns an empty string
• my $format = uploadInfo($file_name)->{'Content-Type'} throws a "Cannot use undefined value as a hash reference" error, even though (like above) $file_name at least has a string value.

I know that the data is getting to CGI.pm (yes, I'm using multipart/form-data on my web page). I know that CGI.pm is at least sort of working, since my original method works fine. My script is using CGI.pm version 2.74 (verified by die ("Version is $CGI::VERSION"); Anyone have the slightest idea of what's going on? Oh yeah, the Perl version is 5.004. Please, someone tell me I'm an idiot.

Thanks bunches!

Gary Blackburn
Trained Killer

[Ovid]

There are a few problems with what I see above. First, it looks like your code is incomplete. Did you try to "pare it down" when posting? Amongst other things, you have while (<$file_name>), but you never opened $file_name.

Problems with the following snippet:

open (F, ">$photo_save_name");

while (<$file_name>) {
    print F;
}

• You should test whether or not "open" was successfful.
• As I mentioned, you didn't open $file_name.
• Further, you haven't reset $/, so after you open $file_name, you'll be breaking $file_name on the newlines.
• while (<$file_name>) should be written as:

  while ( read( $file_handle, $buffer, BUFFER_SIZE ) ) {
  

Those are the major things that I see, so perhaps you can work on those, first.

Incidentally, I share your pain about CGI.pm and file uploads. One thing you might want to try is a new install and ensure that you've installed ALL of the modules with it. Not upgrading all of them can have unpredictable results (I speak from experience).

Cheers,
Ovid

Update: Of course he didn't open $file_handle. He didn't need to. I've written enough upload scripts that I should have paid attention. I am so embarrassed. :)

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

[chipmunk]

Trimbach did not open $file_name because the CGI module actually handles that part:

       CREATING A FILE UPLOAD FIELD

        ...

       When the form is processed, you can retrieve the entered
       filename by calling param().

              $filename = $query->param('uploaded_file');

       ...

       The filename returned is also a file handle.  You can read
       the contents of the file using standard Perl file reading
       calls:

               # Read a text file and print it out
               while (<$filename>) {
                  print;
               }

               # Copy a binary file to somewhere safe
               open (OUTFILE,">>/usr/local/web/users/feedback");
               while ($bytesread=read($filename,$buffer,1024)) {
                  print OUTFILE $buffer;
               }

        ...

There's also an example on checking the mime type:

        ...

       When a file is uploaded the browser usually sends along
       some information along with it in the format of headers.
       The information usually includes the MIME content type.
       Future browsers may send other information as well (such
       as modification date and size). To retrieve this
       information, call uploadInfo().  It returns a reference to
       an associative array containing all the document headers.

              $filename = $query->param('uploaded_file');
              $type = $query->uploadInfo($filename)->{'Content-Type'};
              unless ($type eq 'text/html') {
                 die "HTML FILES ONLY!";
              }

[Trimbach]

I didn't open $file_name because my $file_name = param('photo'); yields both a string with the uploaded filename AND an open filehandle to the uploaded file. Explicitly opening it isn't necessary (and it works as-is.) The only thing I pared down from the posted code was the error-handling and name-generating parts of the code.

Testing the open (F, ">$photo_save_name");, although a good idea, isn't a problem. This is the code that works, remember.

My new code does replace while (<$file_name>) with while ( read( $file_handle, $buffer, BUFFER_SIZE ) ) { (but, really, with 30K files and a 16K buffer I'm not really gaining much) but again, that's not what's giving me fits (none of those things break anything.) Why doesn't $CGI::POST_MAX work? Why doesn't upload(), tmpFileName(), and uploadInfo() work? Sigh. And yeah, I checked my CGI installation and installed all the subclasses before posting. Again, sigh.

Gary Blackburn
Trained Killer

[chromatic]

I've also had trouble with this. There's a bit of disconnect between the docs I've read and the behavior I've actually managed to get working. Here's my test code that simply displays the image to the browser:

#!/usr/bin/perl -wT
use strict;
use CGI qw (:standard);
use CGI::Carp qw( fatalsToBrowser );

my $filename;
if (defined($filename = param('filename'))) {
    my $upload = upload('file');
    print header(-type=>uploadInfo($upload)->{'Content-Type'});
    print while <$upload>;
} else {
    print header();
    print <<HTML;
<html><body bgcolor="#ffffff">
<form method="POST" enctype="multipart/form-data">
<input type="text" name="filename"><p>
<input type="file" name="file"><p>
<input type="submit">
</form>
HTML
}

The big difference in this code is that I used the name of the upload parameter instead of the filename when calling upload(). Contrary to what the Mouse book says, this works for me.

For what it's worth, I'm using CGI.pm 2.68, so this may have changed slightly in a newer version. The only mention of uploads in the ChangeLog is of a one-character bug squashed in 2.74. Your mileage may vary.

[dws]

My upload script does pretty much the same thing, with one tweak to handle newline conversion when uploading text from a Wintel box. (My ISP has CGI.pm 1.45, and I haven't had noticed any problems uploading.)

my $stripCRs = param('filetype') eq 'text';
...
while ( <$upload> ) {
    s/\r// if $stripCRs;
    print UPLOAD $_;
}

Getting a pair of radio buttons on the upload form for 'text' and 'binary' is left as an exercise.

[Trimbach]

I tried your test script and it works perfectly on my server, but after changing my code a bit to reflect yours it still wasn't working. So... the problem wasn't with CGI, but rather with something I did somewhere else in my script.

After some investigation I discovered that apparantly at some point in my development (remember, this is old code) I switched from the OO-style CGI interface to the functional interface, with about 99% using param(blah) and the rest using $object->param(blah). CGI.pm apparantly doesn't have a problem when you mix interfaces for most functions (my HTML-generation and parameter fetching all worked fine) but it apparantly hoses the magic upload functions. Imagine that. :-D Once I took out $object = new CGI; everything miraculously started working.

Remember what I said at the beginning? Well, I'm an idiot. Although I didn't intend to, the lesson here is obvious: don't mix your interfaces in CGI.pm.

Thanks to all who took the time to post... I sure do appreciate it!

Gary Blackburn
Trained Killer

[markjugg]

Hmm. I wonder if this was at the core of the uploadInfo issue. I just got stuck in the same spot with the hash ref error and tried using 100% OO and 100% functional, and I got the same error both ways. Is it possible some files just wouldn't have a mime type associated with them?

update: I have to take that back: I found a "$q = new CGI" in an included module which I think was related. When I used this existing query object to call uploadInfo, it started working.

-mark

[Fastolfe]

Are you changing the value of $file_name between the time you get it and the time you start trying to use it? Attempting to normalize a filename, for example, would goof it up.

Also, are you setting $CGI::POST_MAX before you create your CGI object (or before you use a :standard function like 'param' for the first time)?

[Trimbach]

#1: No, $file_name isn't getting changed, so that (literally):

my $file_name = param('photo');
my $format = uploadInfo($file_name)->{'Content-Type'};

yields "Can't use an undefined value as a HASH reference at testscript.pl line 274." Comment out the $format line and the rest of the script works perfectly with no other modification.

#2: Moved the CGI::POST_MAX to the top of my script and it's now limiting upload size like it's supposed to. Yeah! Thanks bunches! Now if I could just get uploadInfo() to work I'd be all set. (Although the upload() function would be nice, I can live without it in a no strict block.)

Gary Blackburn
Trained Killer

[chipmunk]

You might try poking around in the CGI object...

my $file_name = param('photo');

use Data::Dumper;

print Dumper($CGI::Q->{'.tmpfiles'});

Even better could be to save the state of the form, submitted from the browser...

use CGI qw(:standard);

open(SAVE, ">test.out") or die "Can't open test.out: $!\n";
save_parameters(SAVE);

...and then use that saved state to run the program in the debugger:

use CGI qw(:standard);

open(SAVE, "test.out") or die "Can't open test.out: $!\n";
restore_parameters(SAVE);

$photo_save_name ="something";
    
my $file_name = param('photo');

# ...

Then you can use all the wonderful features of the debugger to figure out why uploadInfo() isn't working as expected. ;)

[damian1301]

Well I don't really have much else to submit to this problem because it was covered by everyother person in the Monestary but I have one suggestion. Why don't you use substring instead of pattern matching to limit it to jpeg. Here is an example that should work...

my $jpeg = 'jpg';
if (lc(substr($upload,length($upload) - 4,4)) eq $jpeg){
    ...do crap    
}

Then after that, you could do a s/// or t/// to replace .jpeg to .jpg, or just tweak with that a little. Best of luck!

Wanna be perl hacker.
Dave AKA damian

[ryddler]

Take a second look at the regex he used.

if ($file_name !~ /\.jpe?g$/)

The way he's using it covers both .jpg as well as .jpeg extensions in case the user tries to upload with either extension. However Trimbach, you might consider adding a "?" after the "g" as well, because as rare as it really is, .jpe is also an acceptable extension used with jpegs.

ryddler

[chipmunk]

On the other hand, /\.jpe?g?$/ would also allow .jp, so this might be more suitable: if ($file_name !~ /\.jp(?:eg?|g)$/)