# Image upload handler with random filename generator and postprocessing

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; }  • 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. Feb 14, 2015 at 10:07 ## 1 Answer 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. • The do-while loop is the way to go, in my opinion. Feb 14, 2015 at 10:14 • @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? Feb 14, 2015 at 10:36
• $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.
– r3wt
Feb 14, 2015 at 10:36
• @janos Is that true? Feb 14, 2015 at 10:41
• 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.
– r3wt
Feb 14, 2015 at 13:14