tigertech

Murmatron 2 wrote:

Page generation renames .expired to .wip (short for ‘write in progress’) which is served by rewrite rules to parallel requests during page generation.

Just to make sure I understand: why is an extra file extension necessary? Can’t you just rename it back to .html and keep the RewriteRules as-is? Then you can skip step 5, too.

While thinking about this, I had a third idea.

The ideal solution would be to make the full WordPress code run just once after a comment is posted, but to do so as soon as possible after the comment (so that the comments aren’t outdated for other visitors).

Perhaps it could be engineered to forcibly make exactly that happen. Something like:

Don’t delete (or touch) the supercached file at all when a new comment is posted. Instead, fire off a separate “fake” standard (non-cookie, non-comment) page load using cURL or something.

That fake page load would use a magic cookie value that mod_rewrite notices to bypass the cache, but WP Super Cache would ignore it and treat the request as cacheable, thus generating a brand new copy of the supercache file.

The overhead is one extra fake page request, but in return, there never needs to be a moment in which the supercached file is missing, so there’s no huge load spike for a few seconds after the comment is posted. Additionally, the delay before the new comment appears for other people is simply the length of a single standard page load — much better than lockdown mode.

donncha wrote:

the script does either a file lock, or semaphore lock to guarantee atomic writes.

Well, the flock/semaphore guarantees exclusive writes, but not atomic ones, unless I’m misunderstanding.

In other words, the flock/semaphore makes sure that only one WP Super Cache process can be writing to the file at once, which is good. But it doesn’t prevent another process — in particular, Apache — from reading from the file while WP Super Cache has half-written it. Apache’s mod_rewrite doesn’t check the flock or the semaphore.

Now that I look at it closely, the problem is not just that fputs() isn’t atomic. There is actually a zero-byte supercached file present on the disk as soon as the WP Super Cache fopen() line creates it, and Apache needs to be prevented from reading that file until after the fputs() returns…. but there’s nothing to prevent Apache from from doing so.

To test that this is a real problem, I shoved a “sleep(30)” line after the fopen() and before the fputs(). Sure enough, that causes an error: if a second request arrives between the fopen() and fputs(), Apache’s mod_rewrite finds the empty file with the “-f” operator, and returns that empty file as the result of the second request. I was actually able to get blank pages to appear in my browser. (If you’re testing this yourself, note that some browsers don’t show zero-length blank pages properly — they just spin for a while without refreshing the page. I had to use Galeon, ApacheBench, or telnet to satisfy myself that it really was a blank page.)

I should emphasize that the sleep isn’t causing the problem — it’s just making it much more likely to happen for any given request by lengthening the crucial interval. The problem must happen occasionally in the normal course of events; it’s just rare.

I can probably supply a patch for this when I get a little more free time over the next couple of days….

PHP ran out of memory.

If you’re using 0.7.1, the line number indicates that it happened while it’s looking through the “supercache” directory. Do you have zillions of files in there? Try deleting them and see if that helps.

While I was envisioning it do the “check and sleep” thing before it makes a database connection, I guess that would be hard to guarantee.

More generally, I agree that the second idea does seem better, because it avoids intentionally delaying anything. Intentional delays would probably just cause something else horrible to happen (even if the database didn’t fill up, the apache process table could easily fill up while waiting).

The version in trunk fixes it, thanks.

Regarding your comment about running PHP scripts under individual UIDs being a security risk: I think there might be a misunderstanding, because it’s actually far more secure than the alternative.

It’s definitely not secure to run the scripts from several different customers under one UID and rely on something like PHP’s safe mode to keep customers separate.

Safe mode, by the admission of PHP’s own developers, was a bad design decision (“It is architecturally incorrect to try to solve this problem at the PHP level”) that hasn’t worked reliably over the years (there have been a constant stream of PHP security vulnerabilities related to it). Safe mode has actually been removed from PHP 6, and the PHP developers are telling people to do it properly with lower-level permissions in the future. Hosting companies will have no choice but to switch if they want to support PHP 6.

Relying on the underlying operating system permissions to keep UIDs separate is inherently more secure, because that code is far better tested and localized. If customer 1000 can only run PHP (and other) scripts under his own unique UID 1000, and that UID cannot access the files and directories of separate customer/UID 1001 due to Unix permissions, that guarantees that one customer can’t read (let alone write) the private files of another customer.

PHP’s safe mode and similar “shared UID” solutions try to accomplish the same thing, but have never worked reliably enough to be truly considered secure (even if the hosting company blocks all other script and shell access, which most don’t).

This change doesn’t seem to be right. With the new version I’m getting a bogus warning telling me to change wp-content to mode 755, even though it’s already mode 755.

This appears to be happening because the code does:

if( is_writable( ABSPATH . 'wp-content/' ) )

… then tells people to chmod 755 if so.

However, decent hosting companies run PHP scripts under the individual user’s unique UID in the first place, meaning that it’s perfectly possible for a mode 755 directory to be writable by the script. In other words, is_writable() doesn’t necessarily mean the directory has been left as mode 777.

If you’re trying to check whether the directory has been left as world-writable on a lame hosting company that uses shared uids (and where leaving it world-writable would therefore be a security risk), which is a good idea, it should probably do something like this instead (untested):

if( fileperms( ABSPATH . 'wp-content/' ) & 0x0022 )

That will tell you if it’s world- or group-writable, I think.

Thanks for the great plugin!