Shared publicly  - 
 
If you are using crypt() in your code you might want to hold off on upgrading to 5.3.7 . I messed up. There will be a 5.3.8 soon.
44
32
Konr Ness's profile photoLorenz Hüsch's profile photoRasmus Lerdorf's profile photoBernhard “Bard” Döbler's profile photo
13 comments
 
We all messed up. Good lesson and let do it better next time :)
 
+Pierre Joye ACK.
And thanks for the quick reaction. PHP development may seem a strange process, but it's a process that works.
 
For people asking me out-of-band what the screw-up was, it was pretty simple. I changed this code:


memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN);
strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1);
strcat(passwd, "$");

to:

memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN);
strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1);
strlcat(passwd, "$", 1);

because the Coverity static analyzer warned about using strcat and we generally try to avoid naked strcat/strcpy in the codebase even though in this case it is safe to do. Of course, unlike strncat(), the 3rd parameter to strlcat() is the maximum length of the destination buffer, not the length of the source string being appended.

The result of this is that if you use an MD5 salt with crypt() you only get the salt back. Non-MD5 salts like DES and Blowfish are unaffected.

No more fixing perfectly working code just before a release!
 
Hi Rasmus, Thanks for the update. Is there a QA test script for something like this?
 
Raymon Irving, yes there are plenty testing this area, and that's the reason why we failed. We did not run the tests before final.
 
Good to see that even pros like +Rasmus make the same mistakes I do. I.e. changing seemingly innocent code before a release.

Not that I am glad it happened but a relief to my inner-critic nonetheless.
 
What is being done to prevent this problem in the future? Is the problem that there are tons of failing unit tests already existing in PHP and thus it is not out of the ordinary to release a new version with failing tests?
 
I'm not all that familiar with PHP Development processes, but don't you guys use some sort of Continuous Integration System that automatically builds runs the unit tests?
We set up Hudson for a rather small project in the third semester with automated tests and all that stuff and the PHP Developers haven't? It's seriously worth the trouble!
 
+Lorenz H.-S. We do. See http://gcov.php.net
You can see the code coverage, test case failures, Valgrind reports and more for each branch.
The crypt change did trigger a test to fail, we just went a bit too fast with the release and didn't notice the failure. This is mostly because we have too many test failures which is primarily caused by us adding tests for bug reports before actually fixing the bug. I still like the practice of adding test cases for bugs and then working towards making the tests pass, however for some of these non-critical bugs that are taking a while to change we should probably switch them to XFAIL (expected fail) so they don't clutter up the test failure output and thus making it harder to spot new failures like this crypt one.
 
Ah, thanks for the clarifiactions! So in this case all that was missing was a list of tests that began failing recently ;)
 
+Lorenz H.-S. Correct, but we shouldn't need that. The plan to address this is to move the persistently failing tests from FAIL to XFAIL and then work through them and either fix/remove those tests or fix the code.
Add a comment...