$query->upload($file)
[download]

should be

$query->upload('upfile')
[download]

From perldoc CGI (my emphasis)

To be safe, use the upload() function (new in version 2.47). When called with the name of an upload field, upload() returns a filehandle, or undef if the parameter is not a valid filehandle.

Thanks adrianh for this - it does make the code cleaner, and make more sense than what I was doing. It's actually something I've tried before but it still dies there though. It's running on apache & the error log just says my script died at the 'upload' line. Weird isn't it?

To debug this change the die to  die $query->cgi_error()

If that is not revealing then have a look at the CGI object which should contain a blessed? reference to the upload file's tmpdir name.

use CGI;
use Data::Dumper;
$q = new CGI;
print $q->header;
print '<pre>' . $q->escapeHTML(Data::Dumper::Dumper($q)) . '</pre>';
[download]

This will prove one way or another if the file is getting to the server and being written to a tmp dir. This may well be the problem. When you do the upload the file is spooled by CGI.pm to a tmp dir ( CGI.pm uses /tmp /temp and something else in that order from memory). If it can't write here it can't spool. Then when you call upload() it will have nothing to deliver and return false, thus causing death as described.

If that fails then try my CGI::Simple module which has the same interface as CGI.pm but far more inclusive and revealing cgi_error() messages. It uses IO::File's tmpfile() method to get a FH for spooling so does not need write permission to /tmp. It also has an addition to the upload() method that lets you do the upload in one line. See the docs for details.

    use CGI::Simple;
    my $q = new CGI::Simple;

    # <INPUT TYPE="file" NAME="upload_file" SIZE="42">

    $files    = $q->upload()                # number of files uploaded
    @files    = $q->upload();               # names of all uploaded fi
+les
    $filename = $q->param('upload_file')    # filename of uploaded fil
+e
    $mime     = $q->upload_info($filename,'mime'); # MIME type of uplo
+aded file
    $size     = $q->upload_info($filename,'size'); # size of uploaded 
+file

    my $fh = $q->upload($filename);         # get filehandle to read f
+rom
    while ( read( $fh, $buffer, 1024 ) ) { ... }

    # short and sweet upload
    $ok = $q->upload( $q->param('upload_file'), '/path/to/write/file.n
+ame' );
    print "Uploaded ".$q->param('upload_file')." and wrote it OK!" if 
+$ok;
[download]

cheers

tachyon

s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

With the code shown, not sure what's wrong. I've just taken your example and, with a few tweaks, the following runs fine on my box:

<FORM ACTION="path_to_form.cgi" METHOD="POST" ENCTYPE="multipart/form-
+data">
<INPUT TYPE="text" NAME="filename" SIZE="10" MAXLENGTH="50">
<INPUT TYPE="FILE" NAME="upfile">
<INPUT TYPE="submit" VALUE="upload">
</FORM>



#!/usr/bin/perl -w

use strict;
use warnings;
use CGI qw(:standard);
use CGI::Carp 'fatalsToBrowser';

$CGI::DISABLE_UPLOADS = 0;
$CGI::POST_MAX = 52428800;

my $SAVE_DIRECTORY = "/tmp";
my $query=new CGI;

my $filename = $query->param("filename") or die "no filename";

# you need to check that the filename only contains sane characters
# otherwise you could have users writing anywhere in the file system
die "bad filename" unless $filename =~ m/^\w+$/;

my $fh       = $query->upload("upfile") or die "no file";

# you realise this could allow multiple users to write to the same
# filename at the same time. probably not what you want.
open(OUTFILE, ">$SAVE_DIRECTORY\/$filename") or die "open failed ($!)"
+;
binmode OUTFILE;
my ($bytes, $buffer, $size);
while ($bytes = read($fh,$buffer,1024)) {
    $size+=$bytes;
    print OUTFILE $buffer;
};
# read can fail with undef - you need to check :-)
die "read failure ($!)" unless defined($bytes);
close(OUTFILE);

print $query->header;
print "$filename uploaded: $size bytes" if $size > 0;
[download]

Try the above on your machine and see if it works

Personally, I would suspect your HTML - it looks like you're not passing upfile for some reason...

The best way of solving this sort of problem is to actually write a smaller version of the code that demonstrates the problem. Once you have your "other code" and "other html" out of the way the error will probably become obvious :-)

open(OUTFILE, ">$UPLOAD_DIR\/$file_name") or die "Upload failed ($!)";

  binmode $fh;
  binmode OUTFILE;

  while ($bytes=read($fh, $buffer, 1024))
  {
    $bytes_read += $bytes;
    print OUTFILE $buffer;
  }
  die "read failure ($!)" unless defined($bytes);
[download]

I think the last line is that bit that made the crucial difference. I didn't know that about 'read'.

I'm glad you've got everything working, but I doubt that the check for read failure was the cause of your problems.

Annoying general advice follows... please feel free to ignore :-)

Unless what you're doing is urgent, I would spend the time and figure out the exact cause of the problem. Take your failing code and add some print statements to find out what's going on, remove code that's not relevant, etc. until the error goes - then back up a step. Look at what caused the error. Try and see why you did it that way - that way you can avoid the problem in the future.

Also, unless you know exactly what caused the error - you don't know 100% that you've fixed it. Maybe there is an edge case somewhere that you're missing?

Learning from your mistakes is a terrible cliche I know, but it's true nevertheless. It's better to learn how to not put the bugs in, rather than learn how to fix the bugs :-)

This is a nit and not related to your question but you're line use CGI qw(:standard); can and should be changed to use CGI;. You don't import all those extra functions when you just use the object interface.

You should also consider using the unambiguous object method calling syntax. Replace $query = new CGI with $query = CGI->new. There are cases where you can introduce unexpected bugs by using your original form. Be nice to yourself and be explicit.

__SIG__
use B;
printf "You are here %08x\n", unpack "L!", unpack "P4", pack
  "L!", B::svref_2object(sub{})->OUTSIDE;
[download]

Diotalevi, Thanks for your advice - I'll take it! I've learned a heap of excellent things here today, and as a bonus, got something working! Thanks again, Stu

Oh you know? I'm a moron. I forgot to mention use strict;use warnings;. This is just good coding hygiene. Add in use diagnostics for verbose warnings. Your code should (in general) run without spitting anything up.

Regards

A.A.