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.
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.my $outputFile; if( $_file_name !~ /^(\s*)$/ ) {
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.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
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.$CGI::DISABLE_UPLOADS = 0; # Temporarily reenable up +loads $CGI::POST_MAX = MAX_FILE_SIZE;
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.
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.# Path and Filename my $file_name = $_file_name;
I would retrieve the basename just before I use it, and certainly after the next block, the one that uses the file type.my $file_type = $query->uploadInfo($file_name)->{'Content-Type'}; + my $basename = basename($file_name);
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.if( $file_type =~ /octet-stream/ ) { $errors{ 'file_type' } = ["","","Unrecognize your submitted re +sume file format."]; goto Print; }
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.$outputFile = $UPLOAD_RESUME_DIRECTORY . $basename ; my $buffer = ""; open(OUTPUT,">>$outputFile");
That not checking the return value of open is a serious mistake should not come as a surprise.
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.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);
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.
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.#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;
Abigail
In reply to Re: strict isn't everything
by Abigail-II
in thread strict isn't everything
by Ovid
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |