Re: strict isn't everything

[Abigail-II]

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*)$/ ) {

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

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;

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;

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);

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;
    }

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");

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);

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;

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

[Ovid]

I guess I don't need to add too much commentary as most of the posts have it fairly well. However, you asked about the rest of the code:

    Print:    
    my $template_data;
    if ( $params ) {
        my @required = qw/ file_size file_type /;
        $template_data = {  
            errors          => \%errors,
            required_fields => \@required,
            # more stuff here
    }
    print $query->header;
    $template->process( 'info_request_emp.tmpl', $template_data ) or d
+ie $template->error();
    exit;
}

The only use of the goto is to skip some code. An else would have been preferred.

As for $file_name and $_file_name, this is a "sometimes" convention used here. When reading form parameters, the variable with the tainted data begins with an underscore and the untainted one drops the underscore. Obviously, if no untainting occurs, this is useless. Personally, I don't like this ad hoc approach.

Cheers,
Ovid

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

[Abigail-II]

The only use of the goto is to skip some code. An else would have been preferred.

Hmmm. An else is a typical solution, but it's not something that makes me jump with joy either. It means there's a large block of code that will be indented. And would you have more such cases, the code crawls to the right hand margin.

A continue might solve this:

    if (CONDITION) {{  # Note the *double* opening brace.
        ... some code ...
        next if SOME_CONDITION;
        ... more code ...
        next if OTHER_CONDITION;
        ... even more code ...
        }
        continue {
            ... end stuff ...
        }
    }

And if you do last instead of next, the continue won't be executed.

Abigail