[wp-trac] [WordPress Trac] #31767: insert_with_markers() is not atomic, leading to corrupted .htaccess updates under race conditions
noreply at wordpress.org
Tue Jun 16 18:07:13 UTC 2015
#31767: insert_with_markers() is not atomic, leading to corrupted .htaccess updates
under race conditions
Reporter: tigertech | Owner:
Type: defect (bug) | Status: new
Priority: normal | Milestone: Awaiting Review
Component: Filesystem API | Version: 4.1.1
Severity: normal | Resolution:
Keywords: has-patch | Focuses:
Comment (by tigertech):
Replying to [comment:5 dd32]:
> I think it'd be safer to simply use `flock()` and/or
`file_put_contents()` with `LOCK_EX` specified.
> Some filesystems won't support locking, but this would cover a bunch of
Right: `flock()` would indeed solve the `.htaccess` corruption problem for
my customers, since we don't use NFS. (I found
some reports] that `file_put_contents()` with `LOCK_EX` can cause complete
write failures on NFS, so I'd stick with `flock()`.)
And it would be a simpler patch, obviously. The reason I didn't suggest
that originally is that some hosting companies do use NFS, and `flock()`
wouldn't fix the problem there.
Even if we use `flock()` to ensure that the `.htaccess` file isn't
corrupted during simultaneous calls to `insert_with_markers()`, there's
still another problem here, even without NFS. Apache may encounter an
empty or partially-written `.htaccess` file after `insert_with_markers()`
truncates it, but before it finishes all the `fwrite()`s. This is not as
bad as a permanently corrupted file, but Apache may temporarily do the
wrong thing with an HTTP request as a result.
A tempfile and atomic rename is the only solution I know of that avoids
both the possibility of corrupted files on NFS, and of Apache occasionally
seeing an empty or partially-written `.htaccess` file.
If we do use `flock()` instead of an atomic rename, I would also still
suggest delaying the file open/truncate and fwrite as long as possible, by
accumulating the data to be written into a variable, then writing it to
the file in a single write at the end, as my patch attempted. This will
minimize the occurrence of the NFS corruption problem and the Apache half-
written file problem, by making the "partially written" duration of the
file as short as possible.
> Really, no system should be writing to the `.htaccess` file often, if
there's a plugin which modifies the rules regularly (especially often
enough to cause corruption at present) I think it should be considered a
bug report for that plugin..
I completely agree that nothing should be doing this. It's horribly
inefficient and pointless. But this still seems like it should be fixed in
WordPress core; two plugins or themes using a documented interface in an
inefficient and pointless manner shouldn't cause data corruption.
I'd be glad to submit a patch that uses `flock()`, together with
accumulating data into a variable to minimize the partially-written file
duration, if that is more likely to be accepted than the previous patch?
Ticket URL: <https://core.trac.wordpress.org/ticket/31767#comment:7>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac