Not checking to see if $_file_name was undef

if( $_file_name !~ /^(\s*)$/ ) {
...}
[download]

If $_file_name is undefined it will be treated as the empty string in this regular expression. The empty string will match the regular expression and the if statement's body will never be executed.

Update:gav^'s very right that using warnings will complain about this. Unfortunately many people ignore their server error logs unless something has gone wrong. (Like me today, when I double checked my above paragraph and it worked as I expected.) So this will work if $_file_name is undefined but only at the expense of creating unnecessary lines in your server error logs. End update. :)

Not sure why the no strict 'refs' is used as nothing is done with symbolic refs in that block.

    # Need binmode or Win32 systems will convert end-of-line chars
    binmode OUTPUT;
    {
        no strict 'refs';
        READ_FILE: while ( read( $file_name, $buffer, BUFFER_SIZE ) ) 
+{
            print OUTPUT $buffer;
            @stats = stat $outputFile;
            last READ_FILE if ( $stats[7] > MAX_FILE_SIZE )
        }
    }
[download]

We're using a string "$file_name" as a filehandle, and so, since it's a string, we are using symbolic references here. perldoc CGI warns us that turning off strict refs will be necessary:

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

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

       However, there are problems with the dual nature of the
       upload fields.  If you "use strict", then Perl will com­
       plain when you try to use a string as a filehandle.  You
       can get around this by placing the file reading code in a
       block containing the "no strict" pragma.

so the writer is doing this part correctly... except they should use -s instead of stat and check that the file opened and all of those good things that have been mentioned.

It should be %errors > 0 rather than just using %errors in boolean context

Using %errors in a boolean context forces a scalar context on %errors. Putting a hash in a scalar context returns the number of buckets used or something like that. If this is non-zero then it will evalute to true. If if it zero, then no buckets have been used, the hash is empty and it will evaluate to false - no errors have been found.

%errors > 0 is equivalent to %errors in a boolean context.

++ to your other points though, especially not santitising the filename before reusing it.

Likewise, if more than one person calls their file "resume" then we'll be adding a mis-mash of resumes on to each other. Ew! And without any kind of file locking, two people could upload their resume.(doc|txt|ps|pdf) and have the files interleaved.

Interesting problem. Too bad that code like this is all too common.

jarich

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.