Here are some of my remarks. I haven't used CGI.pm in half a dozen years so I won't comment much on proper use of its API. I'd also like to point out that some remarks will be subjective - things I would do different aren't necessarely done wrong here.

my $outputFile;
if( $_file_name !~ /^(\s*)$/ ) {
[download]

I would use $_file_name =~ /\S/ here, but it very well may be that something else should be used. Perhaps a if (defined $_file_name) was called for, but the context isn't known.

    use constant BUFFER_SIZE   => 16_384;    # Amount of upload file t
+o read at
one time
    use constant MAX_FILE_SIZE => 3_145_728; # This is the filesize up
+load limit
[download]

Two things. First, the lines exceed 80 characters, which, IMO, is a big no-no. Second there is no point in putting the use statement inside a block - the functions will be exported to the current package. It suggests this has an effect only for this scope, or that it's a run time effect. None of it is true.

    $CGI::DISABLE_UPLOADS      = 0;          # Temporarily reenable up
+loads
    $CGI::POST_MAX             = MAX_FILE_SIZE;
[download]

Unfortunally, not the entire "then" part is given (is just the closing brace missing, or is there more code?), so it's hard to say if something is wrong here. $CGI::DISABLE_UPLOADS is never set back, but that may be in the code that is missing. However, the previous value isn't stored. I would prefer to use local here, to make sure it's set back.

But there is another more serious problem. CGI.pm processes its input when CGI -> new is called. And that's when you need to know when file uploads are enabled or not. Hence, this setting comes to late, it should be done before the $query object is created. And this is true of $CGI::POST_MAX as well.

    # Path and Filename
    my $file_name = $_file_name;
[download]

Having two variables with almost identical names can be confusing, specially if one is a copy of the other. But why use two variables here? $file_name isn't being modified.

    my $file_type = $query->uploadInfo($file_name)->{'Content-Type'}; 
+ 

    my $basename = basename($file_name);
[download]

I would retrieve the basename just before I use it, and certainly after the next block, the one that uses the file type.

    if( $file_type =~ /octet-stream/ ) {
        $errors{ 'file_type' } = ["","","Unrecognize your submitted re
+sume file
format."];
        goto Print;
    }
[download]

I wonder whether the goto is the correct approach here, but since we don't see what's there, I'd give the author the benefit of doubt. I also don't know the role of %errors here, so no comment on that.

    $outputFile = $UPLOAD_RESUME_DIRECTORY . $basename ;
    my $buffer = "";   
    open(OUTPUT,">>$outputFile");
[download]

Ouch. $basename is tainted. I don't see any direct security danger here (due to the basename and the >>) but it makes me feel uneasy. But submits from different people can easily go into the same file - it all depends what filename their browser submitted.

That not checking the return value of open is a serious mistake should not come as a surprise.

 
    my @stats;

    # 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 )
        }
    }
    close(OUTPUT);
[download]

Ok, lots to say about this. What's that no strict 'refs' doing there? I don't think it's doing any harm, but it's odd. And what's the deal with the label on the loop? Again, no harm, but odd. The read is more serious. It's first argument should be a filehandle, not a name of a file. A filehandle to the file that contains whatever is uploaded could be gotten with the upload method.

The OUTPUT handle hasn't been locked so if more than one request is dealt with simultaneously, uploaded data can become interleaved.

Also, OUTPUT is a buffered filehandle. Their might be more data written to it than returned by stat. Finally, the return value of close isn't checked. A full disk can cause a failure of the close.

    #check the file size
    if ( $stats[7] > MAX_FILE_SIZE || %errors ) {
        $errors{'file_size'} = ["","","Your submitted file's size is o
+ver 3MB."]
;
        unlink $outputFile;
[download]

Most likely the author wanted keys %errors here, not %errors. Also, if looks like that if there are already errors, the submitted file is also over 3Mb. Furthermore, if some someone else has submitted a file of 2.9Mb, and we submit a file of 500kb which happens to have the same name, our error message will be that our file size exceeds 3Mb. Which doesn't seem to be correct. Also, the return value of the unlink isn't checked. And again we have timing problems. Another instance might try to write to the file that's being unlinked.

Abigail

---

---

• 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.