homepage Welcome to WebmasterWorld Guest from 50.17.66.61
register, free tools, login, search, pro membership, help, library, announcements, recent posts, open posts,
Become a Pro Member

Home / Forums Index / Code, Content, and Presentation / PHP Server Side Scripting
Forum Library, Charter, Moderators: coopster & jatar k

PHP Server Side Scripting Forum

    
PHP File writing and avoiding a race condition
A little code audit...
mipapage

WebmasterWorld Senior Member 10+ Year Member



 
Msg#: 3165926 posted 9:42 am on Nov 23, 2006 (gmt 0)

So, PHP has this issue where you cannot lock a file until you open it, which leaves the door open for a race condition when writing files. I wrote the following code a while back and think it should work (and in fact use it and it works), I just thought I'd post it here for an audit and in case it might be useful for someone to use or I missed something!

/**
* Write a file to the server.
*
* First open a temp file for writing and acquire a lock.
* Proceed to write, unlock and copy the file from a temp file. If the
* file size is the same, the write worked, clean up and go home. if
* not, clean up and hope it works the next time.
*/
function cms_writeFile($filename, $tempfile, $data) {
$ft = fopen($tempfile, 'w');
if(flock($ft, LOCK_EX)) {
fwrite($ft, $data);
flock($ft, LOCK_UN);
fclose($ft);
if(copy($tempfile, $filename) && filesize($tempfile) == filesize($filename)) {
unlink($tempfile);
return true;
} else { // The whole process failed.
unlink($tempfile);
unlink($filename);
return false;
}
} else return false;
}

I pass it a random tempfile name, so at that point I think we avoid any race condition issues, but to be paranoid I check the file size after the copy. If the files are of a different size, I drop them both and hope this works the next time.

An example to use the function:

if(cms_writeFile('test.php', 'randomg-temp.php', 'some data')) {
echo 'written';
} else {
echo 'notwritten';
}

Seems right, and it works, am I missing anything?

 

mipapage

WebmasterWorld Senior Member 10+ Year Member



 
Msg#: 3165926 posted 9:47 am on Nov 23, 2006 (gmt 0)

Gah, right off the bat I'm seeing that I would do better to gen the tempfile name in the function itself, so as to save doing that every time I call the function...

mipapage

WebmasterWorld Senior Member 10+ Year Member



 
Msg#: 3165926 posted 11:07 am on Nov 23, 2006 (gmt 0)

function cms_writeFile($filename, $data, $tempfile = '') {
if($tempfile == '') {
$tempfile = dirname($filename).'/_'.rand(00000,99999).rand(00000,99999).basename($filename);
}

So that replaces the top of the function now. You can pass it a tempfile if you want, otherwise it takes care of that for you...

dragonthoughts

5+ Year Member



 
Msg#: 3165926 posted 9:34 am on Nov 25, 2006 (gmt 0)

Your function avoids a race condition, but does not prevent two copies of it overwriting the same destination file.

It also does not account for the possibility that two editions of the function write files of the same size.

if two calls pass the same tempfile name, then you are back to the original race condition that you were seeking to solve.

Also, if you used rename() instead of copy() you would have less clean up to do.

it is not necessary to roll your own tmp file name as tempnam ( string dir, string prefix ) does this for you.

There is a standard algorithm that can be used to get around your problem, and prevent file overwriting without locks.

if we assume you have a file "lockfile" that already exists and is locked as if it was a mutex, you could get:

[pre]
function cms_writeFile($filename, $tempfile, $data, $mutex='cms_writeFile') {
$retval=false; //assume failure of function
if(! $tempfile) {
$tempfile = tempnam(dirname($filename),basename($filename));
}
$fm=fopen($mutex, 'w');// existing file that is always present to be locked as a mutex
if (flock($fm), LOCK_EX) { //Use the mutex to ensure only one write is happening
$ft = fopen($tempfile, 'w');
if(flock($ft, LOCK_EX)) { //Now not essential, but still good practice
fwrite($ft, $data);
flock($ft, LOCK_UN);
fclose($ft);
if(rename($tempfile, $filename)) { // check for size became unnecessary
$retval=true; // The only path to success
} else { // The whole process failed.
unlink($tempfile);
}
}
flock($fm,LOCK_UN); // Only unlock mutex when whole atomic action has completed.
fclose($fm);
}
return $retval;
}
[/pre]

This could be trimmed to eliminate the temp file, and in this case, additional locks.
If the revised function is being used to write many different files, then it will form a bottleneck, like a synchronised function in Java.

mipapage

WebmasterWorld Senior Member 10+ Year Member



 
Msg#: 3165926 posted 8:23 am on Nov 27, 2006 (gmt 0)

Thanks for the reply dragonthoughts,

1. rename - thanks (must read more manual).
2. tempnam - thanks (see #1!).
3. If the revised function is being used to write many different files, then it will form a bottleneck

So, this being a common pattern for file locking, are people accepting of this being a bottleneck?

If I was to use this to cache queries, for example, it could theoretically be much slower for a given request with many queries, no?

Also, looking at the function, while the mutex is locked any requests to cms_writeFile will return false and nothing will be written. So for a given request, multiple calls to cms_writeFile won't queue or fire, they will simply die silently, no?

Global Options:
 top home search open messages active posts  
 

Home / Forums Index / Code, Content, and Presentation / PHP Server Side Scripting
rss feed

All trademarks and copyrights held by respective owners. Member comments are owned by the poster.
Home ¦ Free Tools ¦ Terms of Service ¦ Privacy Policy ¦ Report Problem ¦ About ¦ Library ¦ Newsletter
WebmasterWorld is a Developer Shed Community owned by Jim Boykin.
© Webmaster World 1996-2014 all rights reserved