There is a race condition in creating the cache file. My read of the code shows it is simply opening the file to write, then writing the contents. There doesn't appear to be any locking to prevent multiple writers, or any attempt to prevent other processes from reading the file while one is writing to the file at the same time.

As it stands, two (or more) processes could be writing to the same file at the same time, resulting in a jumbled mess, or another process could try to serve up the cached file before it is fully written. The simple test of testing if the file exists before creating it is insufficient because the two steps are not atomic.

The simplest thing to do would be to write the file with the PID appended to the name, then rename it to the actual file name after successfully closing it for writing. This way, even if multiple processes try to create the same cache file, it will always be correct (never will two writers try to write the same physical file), and readers will never see a partially written file as it is atomically put in place by the rename.

CommentFileSizeAuthor
#13 boost-223610.1.patch3.42 KBmikeytown2
#12 boost-223610.patch3.13 KBmikeytown2
#11 boost_racecondition_223610.patch933 byteschristefano

Comments

vivekkhera’s picture

vivekkhera commented

I went ahead and made a patch to make it atomic as described above.

Index: boost.api.inc
===================================================================
--- boost.api.inc	(revision 180)
+++ boost.api.inc	(working copy)
@@ -134,8 +134,14 @@
   if (($filename = boost_file_path($path))) {
     _boost_mkdir_p(dirname($filename));
     if (!file_exists($filename) || boost_file_is_expired($filename)) {
-      if (file_put_contents($filename, $data) === FALSE) {
-        watchdog('boost', t('Unable to write file: %file', array('%file' => $filename)), WATCHDOG_WARNING);
+      $tempfile = $filename . getmypid();
+      if (file_put_contents($tempfile, $data) === FALSE) {
+        watchdog('boost', t('Unable to write file: %file', array('%file' => $tempfile)), WATCHDOG_WARNING);
+      } else {
+        // put the temp file in its final location
+        if (rename($tempfile,$filename) === FALSE) {
+          watchdog('boost', t('Unable to rename file: %file', array('%file' => $tempfile)), WATCHDOG_WARNING);
+        }
       }
     }

I don't know how to make an entry into the translation table for this new error message.

Greg Go’s picture

Greg Go commented

Thanks for contributing the patch! Applied and works.

+1

akalsey’s picture

akalsey commented
Status: Active » Reviewed & tested by the community

This patch is working for me. Marking as reviewed and tested.

drupdrips’s picture

drupdrips commented

+1 works for me too and is a critical patch in my opinion.

Arto’s picture

Arto commented
Title: race condition in creating file » Race condition in cache file creation
Component: Code » Caching logic
Assigned: Unassigned » Arto

Looking into this.

Arto’s picture

Arto commented

Considering using the LOCK_EX argument to file_put_contents() (but see PHP bug #43182) or just basing the locking on flock(). Thoughts?

moshe weitzman’s picture

moshe weitzman commented

@arto - that bug has long since been fixed according to that page. I'd say that file_put_contents() with LOCK_EX is a reasonable solution. You *might* want to declare php 5.1 as a requirement in the module since thats when LOCK_EX became available.

Arto’s picture

Arto commented

@Moshe, well, that PHP bug was apparently fixed. There is no resolution on file other than the automatic "fixed in CVS" at the end (so who knows which option was chosen or what was fixed, exactly), and the version number on the ticket states PHP 5.2.4 in any case, which throws some doubt about relying on consistent LOCK_EX behavior prior to that.

Also, while I've declared PHP 5.2+ a dependency on all my other modules, I'm somewhat more loath to do so with Boost as the module is most generally useful exactly to people who are running their sites on crappy web hosts. In other words, I need to think about this some more... maybe we should go with the original poster's patch for the time being.

moshe weitzman’s picture

moshe weitzman commented

The original patch is fine with me as well.

Arto’s picture

Arto commented

OK, will test it and commit...

christefano’s picture

christefano commented
Version: 5.x-1.0 » 5.x-1.x-dev
StatusFileSize
new boost_racecondition_223610.patch933 bytes

Rerolled for 5.x-1.x-dev.

mikeytown2’s picture

mikeytown2 commented
Version: 5.x-1.x-dev » 6.x-1.x-dev
Assigned: Arto » Unassigned
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new boost-223610.patch3.13 KB

Updated for 6.x as it is still an issue. Improved watchdog reporting so admin has a better chance of fixing the error if it's a reoccurring issue. Split the file writing off in its own function.

mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new boost-223610.1.patch3.42 KB

added more reporting

mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Fixed

committed with a slight fix to the watchdog call in the above patch
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/boost/boost...

lapith’s picture

lapith commented

I have a question about this topic as we have noticed the same conditions on both our boost D5 and D6 installs.

From what I understand that the LOCK_EX argument applies an exclusive lock on the file, closing it to reading and writing. My question is what happens when a script tries to read or write to that file while locked?

For instance, if a page is requested and the cache is being written at the exact time that a second page request is made, would Apache try to serve up a locked file, thus sending an error to the end user?

Also what if 2 concurrent page requests are made and both try to write at close to the same time. Does the process that took slightly longer error out when it tries to write?

It seems like ideally, any process that comes across a locked file waits for the file to be unlocked, but I have a feeling that is wishful thinking. Has anyone noticed either of the scenarios, or have a clear understanding of the inner workings of the LOCK_EX argument?

lapith’s picture

lapith commented

I just looked through the patch, it looks like everyone has decided to go with the create a file then rename it method. Works for me.

mikeytown2’s picture

mikeytown2 commented

I made some very slight changes to the patch, that is now in
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/boost/boost...
in short @unlink($tempfile); when rename fails.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mikeytown2’s picture

mikeytown2 commented
Version: 6.x-1.x-dev » 5.x-1.x-dev
Status: Closed (fixed) » Active

http://drupal.org/node/223610#comment-1138711

mikeytown2’s picture

mikeytown2 commented
Status: Active » Needs review
mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Closed (fixed)

Closing all 5.x issues; will only reevaluate if someone steps up #454652: Looking for a co-maintainer - 5.x

Reason is 6.x has 10x as many users as 5.x; also last 5.x dev was over a year ago. The 5.x issue queue needs to go.