[wp-trac] [WordPress Trac] #31767: insert_with_markers() is not atomic, leading to corrupted .htaccess updates under race conditions

[WordPress Trac]

#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
 them.

 Right: `flock()` would indeed solve the `.htaccess` corruption problem for
 my customers, since we don't use NFS. (I found
 [https://www.google.com/search?q=file_put_contents+LOCK_EX+%22exclusive+locks+are+not+supported+for+this+stream%22
 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