2
\$\begingroup\$

I have a function to handle image uploads. Originally, it contained a goto:

restartA:
$name = $this->random_name() . $ext;
if(file_exists($this->img_path . $name)){
    unset($name);
    goto restartA; 
}

It seems pretty clear to me what the code does, and I think anyone who reads it would immediately understand what's happening. On the other hand, goto is considered evil, so I rewrote it like this:

while(true){
    $name = $this->random_name() . $ext;
    if(file_exists($this->img_path . $name)){
        unset($name);
    }else{
        break;  //weve got a unique name
    }
}

Here's the context in which the code excerpt above appears. In place of some code which I am not allowed to post, I have substituted an array of 3 static sizes.

public function original_img_upload()
{
    $img_name = 'original_image';
    $image_name = $_FILES[$img_name]['name'];
    $image_temp = $_FILES[$img_name]['tmp_name'];
    $image_type = $_FILES[$img_name]['type'];
    $image_info = getimagesize($image_temp);
    $valid = ['.jpg', '.jpeg', '.gif', '.png'];
    $exif = [IMAGETYPE_GIF,IMAGETYPE_JPEG,IMAGETYPE_PNG];
    $ext = strtolower(strrchr($image_name, "."));
    $names_to_return = []; //image names to return to main scope
    $sizes = [
        [1000,608],
        [657,400],
        [329,200]
    ]; //image target sizes

    if( !is_uploaded_file($_FILES[$img_name]['tmp_name']) ){
        $this->e('bad file input');
    }
    if( !in_array(exif_imagetype($image_temp),$exif) && in_array($ext,$valid) ){
        $this->e('file type not allowed');
    }
    if( !$image_info ){
        $this->e('invalid image size.');
    }

    switch($image_type){
        case 'image/png': 
            $image_res = imagecreatefrompng($image_temp); 
        break;
        case 'image/gif': 
            $image_res = imagecreatefromgif($image_temp);
        break;           
        case 'image/jpeg': 
        case 'image/pjpeg': 
            $image_res = imagecreatefromjpeg($image_temp); 
        break;
        default:$this->e('bad file input');
    }

    for($i=0;$i <3; $i++){
        $canvas = imagecreatetruecolor($sizes[$i][0],$sizes[$i][1]);//width + height
        //( resource $dst_image , resource $src_image , int $dst_x , int $dst_y , int $src_x , int $src_y , int $dst_w , int $dst_h , int $src_w , int $src_h )
        imagecopyresampled(
            $canvas,
            $image_res,
            0,0,0,0,
            $sizes[$i][0],
            $sizes[$i][1],
            $image_info[0],
            $image_info[1]
        );

        //create random destination file name
        while(true){
            $name = $this->random_name() . $ext;
            if(file_exists($this->img_path . $name)){
                unset($name);
            }else{
                break;  //weve got a unique name
            }
        }

        $destination = $this->img_path . $name;

        //write image
        switch(strtolower($image_type))
        {
            case 'image/png':
                imagepng($canvas,$destination);
                break;
            case 'image/gif':
                imagegif($canvas,$destination);
                break;          
            case 'image/jpeg':
            case 'image/pjpeg':
                imagejpeg($canvas,$destination,100);
                break;
        }

        //push image name to return images
        array_push($names_to_return,$this->args['base_url'].'img/'. $name);
        //unset resources and free memory
        unset($canvas,$name,$destination);
    }
    return $names_to_return;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ We can review the code that you posted, not the code that you had in mind but decided not to post. I have therefore removed allusions to complications that I don't see in the code. \$\endgroup\$ – 200_success Feb 14 '15 at 10:07
3
\$\begingroup\$

Extract Code to Function and avoid goto

First of all, I would extract this code to it's own function, and then I would use a while loop looking something like this:

private function getUnusedRandomFileName($extention) {
    $name = $this->random_name() . $extention;

    while(file_exists($this->img_path . $name)){
        $name = $this->random_name() . $extention;
    }

    $return name;
}

To avoid the two duplicate lines, you could also use do-while:

private function getUnusedRandomFileName($extention) {
    do {
        $name = $this->random_name() . $ext;
    } while (file_exists($this->img_path . $name)); 
    return $name;
}

Or, since you now have a function, you could return:

private function getUnusedRandomFileName($extention) {
    while(true){
        $name = $this->random_name() . $ext;
        if(!file_exists($this->img_path . $name)){
            return $name;
        }
    }
}

Or use recursion:

private function getUnusedRandomFileName($extention) {
    $name = $this->random_name() . $extention;
    if (file_exists($this->img_path . $name)) {
        return getUnusedRandomFileName($extention);
    } else {
        return $name;
    }
}

So there are many methods to solve this without goto. I like the third best, but choose the one you like.

Misc

  • More functions: in addition to the getUnusedRandomFileName function, I would also add a createThumbnail function, and possibly createImageFrom and writeImageTo, for the first and second switch statement.
  • Paths: at one point you have $this->img_path . $name, and at another you have $this->args['base_url'].'img/'. $name. Do these mean the same thing? Is $this->img_path the same as img? In that case use it everywhere, otherwise it will cause bugs when changed.
  • The image type is completely user controlled, so $image_type = $_FILES[$img_name]['type']; will be whatever the user wants it to be; exif_imagetype is also bypassable. So it seems that the in_array($ext,$valid) is currently the only part that prevents attacks. I would add a comment stating this, just in case the code is changed later on.
|improve this answer|||||
\$\endgroup\$
  • 2
    \$\begingroup\$ The do-while loop is the way to go, in my opinion. \$\endgroup\$ – 200_success Feb 14 '15 at 10:14
  • \$\begingroup\$ @200_success The while-do loop has the advantage of keeping the $name variable within the scope of the loop. Or would this be considered a religious debate? \$\endgroup\$ – janos Feb 14 '15 at 10:36
  • \$\begingroup\$ $this->args['base_url'].'img/'.$name would be the path to the image publically accessible from the internet. $this->img_path . $name is the path to the writeable image folder in relation to the script path. one is to be stored in the database for the front end app, the other is to be used for the script to write the file to the location. \$\endgroup\$ – r3wt Feb 14 '15 at 10:36
  • \$\begingroup\$ @janos Is that true? \$\endgroup\$ – 200_success Feb 14 '15 at 10:41
  • 1
    \$\begingroup\$ I'm just looking at this again on mobile, and I noticed i made a logical error in the file extension check. Should be '|| !in_array..' Instead of '&& in_array..'. Damn no backticks on phone, can't highlight ocde blocks. \$\endgroup\$ – r3wt Feb 14 '15 at 13:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.