Vet my thumbnail generation code (PHP + GD)

SiD the Turtle

SiD the Turtle

SiD the Turtle

Soldato
Joined
16 Nov 2003
Posts
9,682
Location
On the pale blue dot
It's getting a pain generating thumbnails beforehand for my website so I've modified my gallery code to generate them on the fly using GD. This code will then be embedded in a Wordpress page so no complaints about not having the full <HTML>, <BODY>, etc. tags.

I think it's all okay as it works but I'm sure I've done something naughty, missed some steps etc. as it's been a while since I've touched any PHP. What do you think? Here is a sample link and the code:

http://www.sidtheturtle.co.uk/chaos/gallery/gallery.php?galname=halo3&repeat=2&width=200

gallery.php
PHP: 
<?php

// Check if a gallery has been specifed, if not exit cleanly.
if (isset($_GET["galname"]))
{
	$galname = $_GET["galname"];
}
else
{
	exit("Error: No gallery specified.");
}

// Check if specified gallery exists, if not exit cleanly.
if (is_dir("./$galname") != TRUE)
{
	exit("Error: Specified gallery does not exist.");
}

// Check if a number of items before a new row is added is specified, if not exit cleanly.
if (isset($_GET["repeat"]))
{
	$repeat = $_GET["repeat"];
}
else
{
	exit("Error: Row repeat value not specified");
}

// Check if a width for the thumbnails is specified, if not exit cleanly.
if (isset($_GET["width"]))
{
	$width = $_GET["width"];
}
else
{
	exit("Error: Thumbnail width value not specified");
}


// Initialise row length counting variable.
$count = 0;

// Produce gallery
foreach (glob("./$galname/{*.jpg}", GLOB_BRACE) as $image)
{
	$count++;

	echo "<a href=\"" . $image . "\">";
	echo "<img src=\"thumbnail.php?width=" . $width . "&image=" . $image . "\">";
	echo "</a>";
	if ($count == $repeat)
	{
		echo "<p>";
		$count = 0;
	}
	else
	{
		echo " ";
	}
}
?>

thumbnail.php

PHP: 
<?php

	if (isset($_GET["width"]))
	{
		$width = $_GET["width"];
	}
	else
	{
		exit("Error: Thumbnail width value not specified");
	}
	if (isset($_GET["image"]))
	{
		$image = $_GET["image"];
	}
	else
	{
		exit("Error: Source image not specified");
	}

	header("Content-type: image/jpeg");
	$sourceimage=imagecreatefromjpeg($image);
	$x = imageSX($sourceimage);
	$y = imageSY($sourceimage);
	$ratio = $x / $width;
	$dX = $width;
	$dY = $y / $ratio;

	$thumbnail=ImageCreateTrueColor($dX,$dY);
	imagecopyresampled($thumbnail,$sourceimage,0,0,0,0,$dX,$dY,$x,$y);
	imagejpeg($thumbnail,NULL,100);
	$imagedestroy($sourceimage);
	$imagedestroy($thumbnail);
?>
 
Last edited:
do you really want to generate the thumbs on the fly every time someone views the page? why not output the thumbs to disk? or do you really want people to be able to choose a custom size?

and with your error checking, why not have default values? if the value doesn't exist or is invalid, set a default? :)
 
marc2003 said:
do you really want to generate the thumbs on the fly every time someone views the page? why not output the thumbs to disk? or do you really want people to be able to choose a custom size?
Click to expand...
My thoughs were at the moment with my current Wordpress theme I can fit two 200px thumbnails side by side but lets say the overall width increases or I change my mind- I might decide to have 3 150px thumbnails. Though of course I could set up the script to generate them all in one go based on my preferences- but I'd have to hide the script or remove it in case someone finds it and runs it?

QUOTE=marc2003 said:
and with your error checking, why not have default values? if the value doesn't exist or is invalid, set a default? :)
Click to expand...
That is a good idea.
 
You're going to kill your server. Write them to disk, and if you're that concerned about changing the width in future just write them as "filename-widthxheight.jpg".

Why do you want users to be able to specify the widths themselves?

You have lots of XSS vulnerabilities, since you're outputting the "width" and "image" variables directly from input.

Why 100% quality JPEGs :confused:
 
My view;

The code is fine but like rob said there are some "vulnerabilities". For example the following will make a 10,000 pixel wide image
(I advise you don't click this link)
http://www.sidtheturtle.co.uk/chaos/gallery/thumbnail.php?width=10000&image=./halo3/1569941-Full.jpg

So a max size and such is a good idea. Personally though, I would have this running on a database of images and would have them all stored on disk. Then if you wanted to change size, you could just look up all the fullsize images from the database and have a script re-generate each thumbnail that way. So much less server load in the long run!

I can see why you're wanting to do it on-the-fly and to be honest it is okay as long as your site is low-traffic. But if you get a sudden spout of traffic, your server will have a fit.

Another quick one... isn't
glob("./$galname/{*.jpg}", GLOB_BRACE)
the same as
glob("./$galname/*.jpg")

..? (I gotta go so don't have the time to test that one myself)
 
Thanks guys, good advice- just what I was after.

robmiller said:
You're going to kill your server. Write them to disk, and if you're that concerned about changing the width in future just write them as "filename-widthxheight.jpg".
Click to expand...
Good idea.

robmiller said:
Why do you want users to be able to specify the widths themselves?
Click to expand...
I don't, but I might change my mind on sizes later. If I couple this with your idea above I could do a check to see if the file exists but then I still leave it open for someone to do another width I don't want and fill up my disk space- so I think I'll stick it in as a constant.

robmiller said:
You have lots of XSS vulnerabilities, since you're outputting the "width" and "image" variables directly from input.
Click to expand...
Strip tags should do the job for this as I'm not touching any SQL at all (reading through your security guide).

robmiller said:
Why 100% quality JPEGs :confused:
Click to expand...
Because I'm lazy and forgot to change it to something less insane :D

furnace said:
My view;

The code is fine but like rob said there are some "vulnerabilities". For example the following will make a 10,000 pixel wide image
(I advise you don't click this link)
http://www.sidtheturtle.co.uk/chaos/gallery/thumbnail.php?width=10000&image=./halo3/1569941-Full.jpg

So a max size and such is a good idea.
Click to expand...
As above, if I use constants coupled with limits I should be good.

furnace said:
Personally though, I would have this running on a database of images and would have them all stored on disk. Then if you wanted to change size, you could just look up all the fullsize images from the database and have a script re-generate each thumbnail that way. So much less server load in the long run!
Click to expand...
Do you mean actually putting the images into a DB as a blob? Wouldn't that be an unnecessary database hit and also mean I couldn't directly access the images in Wordpress without going back to the database?

furnace said:
Another quick one... isn't
glob("./$galname/{*.jpg}", GLOB_BRACE)
the same as
glob("./$galname/*.jpg")

..? (I gotta go so don't have the time to test that one myself)
Click to expand...
Yes, but I left that in as once I'm happy with the code I'll be adding in extra bits for GIF, PNG etc.
 
SiD the Turtle said:
Do you mean actually putting the images into a DB as a blob? Wouldn't that be an unnecessary database hit and also mean I couldn't directly access the images in Wordpress without going back to the database?
Click to expand...
No no no definitely not :P I don't like the idea of actually having the images in the database. I should've been clearer - I mean storing the URL's to the images in a database, then passing image IDs rather than image URLs. I can explain further if you want but I won't unless you actually ask me to (effort :p)

SiD the Turtle said:
Yes, but I left that in as once I'm happy with the code I'll be adding in extra bits for GIF, PNG etc.
Click to expand...
Gotcha :cool: Just thought I'd point it out!
 
furnace said:
I can explain further if you want but I won't unless you actually ask me to (effort :p)
Click to expand...

Yup I've got you.

What I have noticed is that I have spent so long on the code and had to rewrite and make safe a lot of the code as well as not be sure that some monkey won't find a way around it... that it would be much quicker to use something like Photoshop or Irfan view to just resize them for me prior to upload :D
 
You must log in or register to reply here.
Back
Top Bottom