How not to fix buffer overflows
This tale of woe is making me rethink whether I want to be running any PHP-based software on my website.
Yes, integer overflows happen to the best of us (even those of us who write popular algorithm textbooks), but I would hope that once one is pointed out, the people maintaining the code would have a clue about how to fix it.
Stuff like “if (size>INT_MAX)…” is funny, but I find it even scarier that someone would think the solution to integer overflow is to store potentially-huge byte counts in variables of type “float”. Which is apparently still being done in top-of-tree PHP.
[Hint: “float” is almost always 32-bit IEEE format with a 24-bit mantissa, meaning it can’t represent any integer larger than 223-1 (8 MB) exactly. And round-off error is the last thing you want when computing how large a buffer to allocate.]
July 16th, 2007 at 8:23 PM
July 16th, 2007 at 8:38 PM
I saw the link in your del.icio.us link, and was aghast. For once my smug prejudices were roundly confirmed.
One thing puzzles me, though. calloc’s parameters are size_t’s, not int’s. The size_t type by its nature is unsigned. A strict compiler should warn about trying to fit a signed quantity in the space of the unsigned one when the bit widths of both are the same. (Conversions like unsigned char to signed int are perfectly OK, though; signed to unsigned conversions are never safe.) Even if PHP does not have unsigned int types at the interpreter level, it would seem like you would want to sanitize the integer parameters higher up in the “chunk” routine (i.e., where negative nunbers are already nonsense). At low levels, you could sanity check memory allocations checking against SIZE_MAX (Google around for it).
[Please delete my first empty comment.]
July 16th, 2007 at 9:51 PM
That reminds me of one time when I was a judge at the local ACM programming contest. One of the questions was to write a program to compute the Nth Fibonacci number - with the twist that N could be pretty large (so F(n) would have been greater than MAX_LONG). One of the teams of course did a naive implementation and when they found they had overflow problems, they switched to using longs - but that still wasn’t enough for some of the test cases, so they switched to doubles. When that failed to work they complained about how our compilers were “broken” and refused to accept that they didn’t get that problem right.
Finally during the awards ceremony (when they staged a little protest) one of the other judges and I explained the difference between magnitude and precision and they were sufficiently shamed/embarrassed.
The fact that PHP makes the same CS101 mistake, however… holy fucking ugh.
July 16th, 2007 at 10:25 PM
With gcc you need to explicitly enable the signed-unsigned-assignment warning … and I wouldn’t be surprised if PHP spews out tons of warnings as it builds (too much code I’ve seen does. I always build with warnings=errors.)
July 17th, 2007 at 2:40 PM
There is no “rethink”, there is only “run screaming in terror”. Seriously, it’s known Bad Software.