code critique for file upload

[cassius]

I've managed to make a little upload script for uploading files to a shared dir on my server where me and my co-workers share media files. Is there anyone who cares to take a look at it and perhaps make some suggestions to things that may be done in a better way?

PHP Code:


if($upload){//submit button in form
    if($fil= addslashes(fread(fopen($data, "rb"), filesize($data)))){
        write_file($data, $fil);//call to function write_file
        header("Location: tradefiles.php?user=".$_SESSION['user']."");
    }else echo "****";
}    

function write_file($data, $fil){
$raw = fread(fopen($data, "r"), filesize($data));
    $fs = filesize($data);
    
$file_dir = ".";
$file_url = "http://xx.xx.xx";
echo "This file was uploaded:<br>";
foreach( $_FILES as $file_name => $file_array ) {
    print "path: ".$file_array['tmp_name']."<br>\n";
    print "name: ".$file_array['name']."<br>\n";
    print "type: ".$file_array['type']."<br>\n";
    print "size ".$file_array['size']."<br>\n";
    move_uploaded_file( $file_array['tmp_name'], $file_dir.'/'.$file_array['name'] )
        or die ("Couldn't Copy");
}
    if (!$handle = fopen($file_array['name'], 'w+')) {
         print "Cannot open file (".$file_array['name'].")";
         exit;
        }
    if (!fwrite($handle, $raw)) {
        print "Cannot write to file (".$file_array['name'].")";
        exit;
        }
    fclose($handle);
}


This script is used on a MS WIN2003 server with IIS and PHP 4.3xx

[Natty_Dreadlock]

if u used $file and not $fil, everything would be fine - actually i do not see any obvious mistakes etc. if u do not have any concrete problem (or feature u'd like to implement) I'd say you can use your code.

[cassius]

Thank you for your reply. I guess why i used $fil instead of $file is just a language bug since $file = $fil in norwegian. I usually use english "names" for my variables, but sometimes I fail to do so all the way through my code.

My code does the work for me, so this post was just to get a critique or alternative suggestions for the way to write the code.

Tnx, cassius

[LordShryku]

I'd suggest coding it so it doesn't require register_globals to be on.
Specifically, this part...
if($upload){

[cassius]

I can see your point here, and to be honest I tried out that part first. I though seem to get an error while trying to "fetch" the file from my file-input(in the form).

ex: I tried this:

PHP Code:

if($_POST['upload']){//submit button in form
    if($fil= addslashes(fread(fopen($_POST['data'], "rb"), filesize($_POST['data'])))){
        write_file($_POST['data'], $fil);//call to function write_file
        header("Location: tradefiles.php?user=".$_SESSION['user']."");
    }else echo "****";
}


But it didn't work(error message: "undefined index: data" or something like that). I also tried to change $_POST['data'] to both $_FILES and to $_HTTP_POST_FILES with the same error message in return.

I would appreciate some help on changing my script to handle register_globals = Off, but that would probably fit better into the "coding" or "newbie" - board.

[bbaassiri]

IMHO having such function calls all in one isn't a good way to determine if an error occured

PHP Code:

if($fil= addslashes(fread(fopen($_POST['data'], "rb"), filesize($_POST['data'])))){


how do you know if the fopen failed? how do you know if fread is no good? you need to do more checking to see what your are doing is sane. For example what if you are reading 100MB file? does it make sense to read at once or in pieces of 10MB ? You should try volume testing your code and see the results. Im pretty sure that you could bring down the box or close to it with a 100MB mp3 file

[cassius]

Thank you for you reply, it sounds like a good idea to do some more checking here...I will look into that in a moment

[mtimdog]

bbaassiri, you'd have to change some configuration variables if you're gonna be uploading a 100 MB file. There are limits so people don't do something as foolish as this. btw, I think that if you really need someone to upload a large file (and you have to authenticate the person) you mise well give them bloody ftp access to one directory all their own (of course the site would have it's own quota limit though).

[bbaassiri]

Originally posted by mtimdog
bbaassiri, you'd have to change some configuration variables if you're gonna be uploading a 100 MB file. There are limits so people don't do something as foolish as this. btw, I think that if you really need someone to upload a large file (and you have to authenticate the person) you mise well give them bloody ftp access to one directory all their own (of course the site would have it's own quota limit though).

I agree that a 100MB file wasn't the correct way of providing an example but really, there are so many different situations developpers should think about when coding. All I'm trying to convey is that configurations, limits , load average, memory usage, network latency, etc should be at the least thought of and that reading all at once without checking if there is a valid file handle isn't what i like to see. There are error codes for a reason for whatever that reason might be. The old saying is never assume because you can make an ASS out U and ME.

[mtimdog]

Exactly, error check everything. I know it's hard when you write something that will never ever break, but what if there's a disk error, or the db gets corrupt.