#39648 [Csd]: Implementation of PHP functions chown() and chgrp() not thread safe.

ID: 39648 User updated by: wharmby at uk dot ibm dot com Reported By: wharmby at uk dot ibm dot com Status: Closed Bug Type: Unknown/Other Function Operating System: Linux RHEL4 PHP Version: 5CVS-2006-11-27 (snap) Assigned To: iliaa New Comment: First thanks for taking a look at this one for me Ilia. One question about the fix though. Rather than unconditionally calling getpwnam_r and getgrnam_r as my patch did you have modified the autoconf input to cause the necessary "HAVE_" variables to be set and #if def'd the code to check these variables. This was the bit I was unsure about as I have not worked with autoconf files much before but that all looks straightforward and will help me construct a fix for a similar issue I have identified in the PHP code. My only concern is that as the code stands if the reentrant (_r) version of the function is NOT available then the standard function is called instead. This assumes that if a library does not implement a "_r" version of the function that the basic function is reentrant. Whilst I don't know of an instance when this is not true I wondered if this was a known safe assumption ? It would sure make life easier if it was true but I have not seen this documented anywhere. You have probably already done all the necessary research and found this to be a safe assumption but for my own future information it would be very useful if you could confirm this please. I half expected to see code similar to that in reentrancy.c where if the "_r" version is not available the standard function is called wrapped in a mutex. I realize that assuming that if a "_r" version of a function is not available that assuming that the standard function must NOT be reentrant could also be a flawed assumption and without extra #if def's in the code could lead to unnecessary single threading of the code but at least it ensures thread safety. Previous Comments: ------------------------------------------------------------------------ [2006-11-29 23:42:14] iliaa@php.net This bug has been fixed in CVS. Snapshots of the sources are packaged every three hours; this change will be in the next snapshot. You can grab the snapshot at http://snaps.php.net/. Thank you for the report, and for helping us make PHP better. ------------------------------------------------------------------------ [2006-11-27 17:05:42] wharmby at uk dot ibm dot com Description: ------------ The current implementation of the chown() and chgrp() functions on Linux use the non-reentrant getpwnam() and getgrnam() C library calls respectively rather than the reentrant getpwnam_r() and getgrnam_r(). Therefore using either chown() or chgrp() on Linux in a ZTS enabled build could lead to unpredictable/undesirable results. The following patch, which was built against the latest snapshot (Nov 27th, 2006, 0730 GMT) modifies the code in ext/standard/filestat.c to use the reentrant versions of these functions and so make these 2 functions thread safe: http://pastebin.ca/259657 However, I am concerned that this patch relies on the C library supporting the POSIX.1 functions getpwnam_r(), getgrnam_r() and sysconf(). These are all implemented by GNU libc but are there other C libraries used to build PHP which may not have the necessary support ? Or is it reasonable to assume that all C libraries used when buiding PHP will be POSIX.1 compliant ? N.B There are other uses of these non-reentrant functions in the PHP code base (e.g posix.c and fopen_wrappers.c) and I m happy to produce the necessary patches to fix these uses if this fix proves satisfactory. Andy Wharmby IBM United Kingdom Limited Reproduce code: --------------- --------------- Problem found by code inspection. As with most thread safety issues difficult to produce a simple testcase which will show a reproducible crash but current Linux executable is clearly not reentrant. Expected result: ---------------- N/A Actual result: -------------- N/A ------------------------------------------------------------------------ -- Edit this bug report at http://bugs.php.net/?id=39648&edit=1