princepawn has asked for the wisdom of the Perl Monks concerning the following question:

Hi, I need some help guys and girls. I am the author of a somewhat popular module called HTTP::File, which one-lines that oft-repeated ritual of storing files uploaded to a website via a form.
Further, by orthogonal use of File::Basename, it performs path extraction from the uploaded file in an architecture independant manner.
However, now a user of my module is having a problem. The raw facts first:
  1. Perl Version 5.60
  2. CGI.pm Version 2.56
  3. HTTP::File Version 3.00
  4. Architecture: Windows NT (dont know what webserver)
  5. When using HTTP::File the following is occurring:
    1. A zero-length file of the correct name and directory shows up on his system... the only problem is its zero-length
    2. The uploaded file shows up in /tmp (c:\temp) as CGItemp39633, with the correct contents.
    3. I am including the source for my upload module below. The only line which is suspect is this one: 
        while ($bytesread = read($raw_file,$buffer,1024)) {
        print O $buffer;
    }

      [download]
      The reason it is suspect is that I do not check if the read is defined and thus both undef and 0 return values will end the loop.
      If you can suggest a means of improving this one line, I will appreciate it. Source code for HTTP::File follows: 
      
#!/usr/bin/perl


use HTTP::Headers::UserAgent;
use File::Basename;


$VERSION = '3.0';

package HTTP::File;
sub platform {

    $__platform = HTTP::Headers::UserAgent::GetPlatform ($ENV{HTTP_USE
+R_AGENT});
    $__platform =~ /^Win/ && return 'MSWin32';
    $__platform =~ /^MAC/ && return 'MacOS';
    $__platform =~ /x$/   && return '';


}

sub upload {

    ($raw_file, $path,)=@_;

#    warn " ** raw_file $raw_file\n";

    $path = $path ? $path : '/tmp';

#    warn " ** path $path\n";

    $platform = platform;

#    warn " ** platform $platform\n";

  File::Basename::fileparse_set_fstype(platform);
      ($basename,$junk,$junk) = File::Basename::fileparse $raw_file;

#    warn " ** basename $basename\n";

    open  O, ">$path/$basename" || die $!;
    while ($bytesread = read($raw_file,$buffer,1024)) {
        print O $buffer;
    }
    close O;

    return $basename;

}


1;

      [download]

Replies are listed 'Best First'.
Re: Windows NT CGI File Upload Problem
by tye (Sage) on Sep 14, 2000 at 19:36 UTC

    Just a quick thought: This can happen if the file starts with a CTRL-Z and you didn't use binmode() under Win32.

    If you don't know that the file(s) you are dealing with will be just text, then you should use binmode(). Unfortunately, binmode() is a no-op under Unix so it doesn't get used much and the fact that this is a problem doesn't get noticed much.

            - tye (but my friends call me "Tye")
[reply]
[d/l]
[select]
      However note that under VMS binmode is not a no-op and is sometimes very definitely not desired. Sorry, I don't know details beyond the fact that it has to do with VMS having a much more complex filestructure than Unix, I was just told that once by the maintainer of the VMS port.
[reply]

        Actually, under VMS (last time a checked, which was quite a while ago), simply writing files from C (and thus from Perl) generates strange files. VMS files are handled by RMS and the "R" stands for "records" so VMS files are not all streams. Until C got ported to VMS, VMS didn't even have support for streamed files (that I could find).

        I could see binmode() creating some kind of raw stream instead of the default "stream, LF-terminated records" format that C creates by default (though I don't think it used to). This might cause normal VMS stuff to puke because "read next record" gives you something huge. But that is already a problem for binary data in a "stream, LF-terminated records" file. Which makes me think that bindmode() should (once again?) be a no-op under VMS. But this is all wild speculation (which often prompts someone who knows something to reply; so I won't delete it).

        But you can probably recover the damage from the use of binmode() under VMS using its standard file conversion utilities. The damage from no binmode() under other OSes usually isn't reversible.

                - tye (but my friends call me "Tye")
[reply]
[d/l]
[select]
(Ovid - Is this a security hole?)RE: Windows NT CGI File Upload Problem
by Ovid (Cardinal) on Sep 14, 2000 at 20:30 UTC
    Okay, maybe I'm missing something here, so any light shed on this would be great.

    If I am not mistaken, you're allowing the user to specify the filename in $basename? Admittedly, File::Basename appears to do a good job of just extracting that name and nothing else. Therefore, specifying a filename of ../etc/passwd probably can't happen. However, I'm wondering if the person sending the data can spoof a different OS which doesn't use / as a path separator, and therefore allow the above path to be used as a filename. Not sure if that's possible or not, but I wouldn't bet against it.

    Also, I can't help but wonder if it's vulnerable to the problem documented here. Just having untainted data going to the shell gives me willies.

    Cheers,
    Ovid

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

[reply]
Re: Windows NT CGI File Upload Problem
by Ovid (Cardinal) on Sep 14, 2000 at 19:44 UTC
    I can't say for sure what your problem is, but the following code has a problem: 
    while ($bytesread = read($raw_file,$buffer,1024)) {
    print O $buffer;
}

    [download]
    $bytesread doesn't do anything and should be skipped. My thought is that if the read doesn't get anything, having that "nothing" assigned to bytesread will cause the conditional to evaluate as true and while will write to the file regardless of the success of read. Try the following: 
    while (read($raw_file,$buffer,1024)) {
    print O $buffer;
}

    [download]
    Please note that if you had used -w, you would have been warned that $bytesread was only used once and this might have been a significant clue.

    Also, I couldn't tell at first if your filehandle was the letter 'O' or a zero. Obviously, it wasn't a zero, but I first thought that maybe it was a type causing an problem. Longer filehandles like OUTPUT are less type prone and much easier to maintain.

    Cheers,
    Ovid

    Update: Thanks to tye for clariying that. I just knew I should have written some test code.

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

[reply]
[d/l]
[select]

      $bytesread doesn't do anything and should be skipped. My thought is that if the read doesn't get anything, having that "nothing" assigned to bytesread will cause the conditional to evaluate as true and while will write to the file regardless of the success of read.

      Yes and no. Yes, $bytesread is useless. No, it doesn't change the meaning of the conditional.

              - tye (but my friends call me "Tye")
[reply]
      Actually in a sense $bytesread should be doing something though... I should be ensuring that it is undef on each iteration of the loop... I think I now see the light. I should rewrite this as: 
      {
 # read() returns the number of bytes read or undef on error
 $bytesread = read($raw_file,$buffer,1024);

 # let's exit with an error if read() returned undef
 die "error with file read: $!" if !defined($bytesread);

 # let's exit with an error if print() did not return true
 unless (print O $buffer) die "error with print: $!";

 # let's redo the loop if we read > 0 chars on the last read
 redo unless !$bytesread
}

      [download]
      And many thanks for one of the very first posts ever from the all-great merlyn which showed me the merits of a naked do block.
[reply]
[d/l]

Back to Seekers of Perl Wisdom