A nasty bug
A few weeks ago a pretty nasty bug appeared on my image hosting service pici.se about a month ago. For some reason, some people started noticing that their thumbnails were changed to pictures which didn’t belong to them. This was a rather serious issue, and I had a look at the code to figure out what was going on. I realized that I had recently changed my deployment from Django under apache to Django under lighttpd and this was surely related to that… but how?
It turned out that it was a combination of bugs that together fucked stuff up for me pretty bad. Since I started allowing pictures over 1MB a while ago, but wanted to limit hotlinking to them, I put them into a separate directory from the other pictures being served. That meant that the large pictures were given filenames such as “large/asSDavXZ.jpg” instead of “asSDavXZ.jpg” where the filename is randomly generated. I had however forgotten to update my code to check for the presence of these images when fetching a new random file name, that is my code looked something like this (pseudo-code):
filename = get_random_stuff() while file_exists(filename): filename = get_random_stuff()
when it should have been changed to this:
filename = get_random_stuff()
while file_exists(filename) or file_exists("large/" + filename):
filename = get_random_stuff()
When I saw this, the first thing that came to mind was that this shouldn’t be such a big deal. Generating two filenames which were the same should pretty much never happen since there are so many possible combinations of characters.
Well… it did. And quite often. Running some scripts on the server showed that there were quite a lot of these collisions which seemed REALLY weird, and then I found this bug in Django. It seems like fastCGI deployment using method=prefork gives the same random seeds to each process. In combination with my fuckup, this made these collisions happen quite a lot and people got their thumbnails overwritten since all thumbnails were stored in the same folder without the “large/” prefix for each image.
That is, for a pictures thumbnail to be overwritten the following had to happen:
1. Someone uploads a picture below 1MB and is handed to fastCGI process 1 and given a “random” string for it’s filename.
2. Someone else uploads a picture larger than 1MB and is handed to fastCGI process 2, and given the same randomized string as a filename due to the random seeds being non-random due to a bug in Django.
3. My code has a nasty bug in it and doesn’t detect this collision.
4. Thumbnails are generated for image2 at the same target as the thumbs for image1.
Fixing the bug in my code was rather trivial but I also patched my Django installation to avoid any other weird issues due to non-random seeds. The current patch which is available for the bug should however not be utilized since it uses time.ctime() as a random seed for each request, and ctime() will only change once a second which means that subsequent requests given to the same fastCGI process during the same second will be given the same seed. Instead time.time would be better, so I patched my installation with pretty much the same thing but using the following instead.
random.seed("%d%f" % (getpid(), time()))
This seems to generate random values for each request as far as my testing goes.
Recent Comments