Implementation of PHP functions chown() and chgrp() not thread safe.

Bug #39648	Implementation of PHP functions chown() and chgrp() not thread safe.
Submitted:	2006-11-27 17:05 UTC	Modified:	2006-11-30 11:27 UTC
From:	wharmby at uk dot ibm dot com	Assigned:	iliaa (profile)
Status:	Closed	Package:	Unknown/Other Function
PHP Version:	5CVS-2006-11-27 (snap)	OS:	Linux RHEL4
Private report:	No	CVE-ID:	None

[2006-11-27 17:05 UTC] 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

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports

[2006-11-29 23:42 UTC] 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-30 11:27 UTC] wharmby at uk dot ibm dot com

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.