ID: 43182
User updated by: chris_se at gmx dot net
Reported By: chris_se at gmx dot net
Status: Open
Bug Type: Streams related
Operating System: Any POSIX-compatible OS
PHP Version: 5.2.4
New Comment:
Oh, I forgot:
There's a solution for this problem that is quite a bit easier as the
other one I suggested:
ALWAYS open the file in append mode and truncate it AFTER acquiring the
lock. Normally this has the drawback that the append mode does NOT have
the same semantics as the write mode (i.e. using fseek to reposition the
write pointer won't work as the contents will always be appended to the
end of the file) but that doesn't matter here since a) with ftruncate()
the end of the file will be at byte zero and b) file_put_contents only
writes out the complete data and does not need to reposition the write
pointer of the file.
Well, at least with normal C and fopen(3) it's not that complicated,
with PHP's streams API it gets complicated again because some stream
wrappers don't support truncating of a file (e.g. the FTP wrapper).
Those don't support locking either, so that wouldn't be a problem anyway
except that it's as far as I could see not possible to determine whether
truncating the file will be possible BEFORE opening it.
Anyway, here's a patch against the PHP 5.2 branch that fixes this bug:
===================================================================
RCS file: /repository/php-src/ext/standard/file.c,v
retrieving revision 1.409.2.6.2.28
diff -u -r1.409.2.6.2.28 file.c
--- ext/standard/file.c 4 Sep 2007 12:51:49 -0000 1.409.2.6.2.28
+++ ext/standard/file.c 6 Nov 2007 18:49:48 -0000
@@ -604,16 +604,44 @@
context = php_stream_context_from_zval(zcontext, flags &
PHP_FILE_NO_DEFAULT_CONTEXT);
- stream = php_stream_open_wrapper_ex(filename, (flags &
PHP_FILE_APPEND) ? "ab" : "wb",
+ // open in append mode and truncate later with LOCK_EX
+ // note that this will cause the stream to be closed and opened
again
+ // if truncating is not supported - but there is no way with
the
+ // current streams API to check before actually opening the
file
+ stream = php_stream_open_wrapper_ex(filename, (flags &
PHP_FILE_APPEND || flags & LOCK_EX) ? "ab" : "wb",
((flags & PHP_FILE_USE_INCLUDE_PATH) ? USE_PATH
: 0) | ENFORCE_SAFE_MODE | REPORT_ERRORS, NULL, context);
if (stream == NULL) {
RETURN_FALSE;
}
+ // worst-case scenario: user wanted LOCK_EX but the stream
can't be
+ // truncated. So, reopen in 'wb' mode and ignore locking
+ // (TODO: possibly generate a warning that LOCK_EX wasn't
possible with
+ // this stream?)
+ if (!(flags & PHP_FILE_APPEND) && (flags & LOCK_EX) &&
!php_stream_truncate_supported(stream)) {
+ php_stream_close(stream);
+ stream = php_stream_open_wrapper_ex(filename, "wb",
+ ((flags & PHP_FILE_USE_INCLUDE_PATH) ?
USE_PATH : 0) | ENFORCE_SAFE_MODE | REPORT_ERRORS, NULL, context);
+ // remove LOCK_EX flag - locking will be useless now
anyway
+ // (we just truncated the file)
+ flags = flags & ~LOCK_EX;
+ }
if (flags & LOCK_EX && (!php_stream_supports_lock(stream) ||
php_stream_lock(stream, LOCK_EX))) {
php_stream_close(stream);
RETURN_FALSE;
}
+
+ // if the file was to be locked and not to be opened in append
mode,
+ // we now need to truncate it (we already checked for truncate
support
+ // and reopened the stream and removed LOCK_EX if it wasn't
available)
+ if (!(flags & PHP_FILE_APPEND) && (flags & LOCK_EX)) {
+ if (php_stream_truncate_set_size(stream, 0)) {
+ // we couldn't truncate the stream even though
+ // truncating was supported
+ php_stream_close(stream);
+ RETURN_FALSE;
+ }
+ }
switch (Z_TYPE_P(data)) {
case IS_RESOURCE:
===================================================================
It is a bit ugly because of the problem described above: It can only be
determined whether it's possible to truncate the file after it was
opened, so if it's not, it has to be closed and opened again. The
function remains functional in any case, just causes a little additional
overhead if a stream wrapper without file truncation support (e.g. FTP)
AND LOCK_EX are used together.
So, you have four possibilities now:
1) Use the patch as-is.
2) Improve the patch by somehow detecting truncate support before
opening the file (and therefore removing the need to reopen the file if
such a wrapper is used in combination with LOCK_EX).
3) Add the already mentioned file mode that doesn't include O_TRUNC
4) Remove the LOCK_EX flag from file_put_contents alltogether.
Previous Comments:
------------------------------------------------------------------------
[2007-11-04 22:11:56] chris_se at gmx dot net
Reopened.
------------------------------------------------------------------------
[2007-11-04 22:11:43] chris_se at gmx dot net
Closed to reopen the bug.
------------------------------------------------------------------------
[2007-11-04 16:12:02] chris_se at gmx dot net
Excuse me, I don't intend to sound rude - but did you even read my
report? I never even mentioned O_EXCL and it has NOTHING to do with the
problem I reported.
To summarize the problem again (and perhaps make myself clearer):
file_put_contents has - according to the documentation - a third
parameter called $flags. In the documentation, it is stated, that the
LOCK_EX constant may be passed as a flag. The documentation for
file_put_contents states:
> LOCK_EX: Acquire an exclusive lock on the file while proceeding to
the writing.
So, one may assume that the file will be locked exclusively BEFORE
writing to it.
Now, the problem is, that this is not the case! Have a look at
ext/standard/file.c:
http://cvs.php.net/viewvc.cgi/php-src/ext/standard/file.c?revision=1.409.2.6.2.27&view=markup
There you see this line of code:
stream = php_stream_open_wrapper_ex(filename, (flags &
PHP_FILE_APPEND) ? "ab" : "wb",
((flags & PHP_FILE_USE_INCLUDE_PATH) ? USE_PATH : 0) |
ENFORCE_SAFE_MODE | REPORT_ERRORS, NULL, context);
Followed by that line:
if (flags & LOCK_EX && (!php_stream_supports_lock(stream) ||
php_stream_lock(stream, LOCK_EX))) {
So, what does this code do? (if FILE_APPEND was not specified)
1) It openes the file in 'wb' mode for writing.
2) It locks the file exclusively
3) THEN it actually starts writing the file contents
So, the problem actually is the following:
'wb' as a fopen(3) mode translates to O_WRONLY | O_CREAT | O_TRUNC as
an open(2) mode. What does that do? It truncates the file UPON OPENING
IT! And AFTER THAT it tries to acquire the lock. Since locks on
POSIX-compatible systems are advisory (flock(2) anyway), any possible
other lock on the file will NOT be honoured by file_put_contents and the
$flag == LOCK_EX parameter is completely ineffective.
Please, have a look at my test case - it's really simple. One PHP
script opens the file for reading, acquires a shared lock and goes to
sleep. If the other PHP script is executed in the mean time, the file is
opened and then the other script tries to acquire an exclusive lock on
the same file - and has to wait until the first script releases it.
That's fine. But what's not fine is that prior to acquiring the
exclusive lock it has ALREADY modified the file! So after the first
script returns from it's sleeping phase, it will see an empty file
because it was truncated by the other script upon opening.
What the CURRENT PHP code translates to is basically:
int fd = open ('file.txt', O_WRONLY | O_CREAT | O_TRUNC, 0666);
flock (fd, LOCK_EX);
Which causes exactly the described problems.
And there IS a solution for this. The following C code CORRECTLY
acquires an exclusive lock for writing to a file WITHOUT truncating it
before it is safe to do so:
int fd = open ('file.txt', O_WRONLY | O_CREAT, 0666);
flock (fd, LOCK_EX);
ftruncate (fd, 0);
// now write something to fd
This is absolutely correct in POSIX and completely portable (assuming
flock(2) is provided by the OS, but if fcntl(2) is used as a
replacement, it does not change anything).
So now the ONLY question remains: How is it possible to integrate that
fix with PHP code?
And that is a bit more tricky than the mere C code becaues PHP uses
fopen(3)-style file modes for the stream wrapper API instead of numeric
file modes as provided by open(2). And there is currently no
fopen(3)-style file mode translating to O_WRONLY | O_CREAT.
So, I see two possible solutions:
* Add another fopen(3) style file mode that does exactly that.
* Remove the LOCK_EX flag from file_put_contents completely.
It DOES NOT make sense to keep the flag but leave this bug unfixed,
because
a) It is utterly and completely useless with advisory locking.
Keeping it will only cause people who read the documentation
to assume it's safe to use the flag - which is plainly WRONG.
b) If mandatory locking is used (i.e. when using Windows), the
OTHER LOCK that is already in place on the file will take care
for the data consistency, i.e. a lock created by another
program will delay the truncating of the file until AFTER the
lock is released. The exclusive lock created on the file itself
will have NO interaction with any other lock in place prior to
the file_put_contents_call.
------------------------------------------------------------------------
[2007-11-04 15:30:54] [EMAIL PROTECTED]
On *nix systems O_CREAT and O_EXCL are mutually exclusive and will
prevent creation of a file if one already exists. Therefore lock needs
to be created separately or you need to create another file on the same
disk and then use atomic rename.
------------------------------------------------------------------------
[2007-11-03 16:45:01] chris_se at gmx dot net
Description:
------------
The LOCK_EX flag of file_put_contents suggests that the function will
use an advisory lock to ensure transaction safety. This is NOT the case
(except when combined with FILE_APPEND). It actually DOES request an
exclusive lock on the file but only does so AFTER opening it in the 'wb'
mode which will truncate the file on opening BEFORE the actual lock can
be acquired.
The correct behaviour would be to open the file for writing without
truncating it, in C for example using
int fileno = open (file, O_WRONLY | O_CREAT, 0666);
(WITHOUT adding O_TRUNC!), THEN acquiring the lock using flock() and
THEN truncating the file to 0 bytes length.
I don't know if there's a simple possibility to integrate it with the
current streams API (since there's no fopen mode that will map to either
O_WRONLY | O_CREAT or O_RWDR | O_CREAT) but if it's not possible to fix
it, you should at least remove the option, since it suggests something
it can't provide with advisory locking.
This is not a problem on Windows since Windows locks are always
mandatory.
Reproduce code:
---------------
First script (start in in a first window using any P):
<?php
file_put_contents ('file.txt', 'Hello World!');
$f = fopen ('file.txt', 'r') or die ("Could not open file!\n");
flock ($f, LOCK_SH) or die ("Could not acaquire lock!\n");
echo "Sleeping for 20 seconds (please use file_put_contents script in
the mean time!)\n";
sleep (20);
$x .= fread ($f, 1024);
fclose ($f);
echo "Contents was: '" . $x . "'\n";
?>
Second script (start it in a second window in the same directory while
the first one is sleeping):
<?php
file_put_contents ('file.txt', 'ByeBye Joe!', LOCK_EX);
?>
Expected result:
----------------
The first script should output:
Sleeping for 20 seconds (please use file_put_contents script in the
mean time!)
Contents was: 'Hello World!'
Actual result:
--------------
The first script outputs:
Sleeping for 20 seconds (please use file_put_contents script in the
mean time!)
Contents was: ''
------------------------------------------------------------------------
--
Edit this bug report at http://bugs.php.net/?id=43182&edit=1
