[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:  dd32
     Type:  defect (bug)           |      Status:  assigned
 Priority:  normal                 |   Milestone:  4.4
Component:  Filesystem API         |     Version:  4.1.1
 Severity:  critical               |  Resolution:
 Keywords:  4.4-early needs-patch  |     Focuses:
-----------------------------------+-----------------------

Comment (by dd32):

 Thanks @kevinatelement @tigertech @willmot

 > Previously insert_with_markers would create the file if it didn't
 already exist, now it simply returns false if the file doesn't exist.

 #oops :) Can't believe I missed that..

 Ideally I guess I'd have used `c+` (PHP 5.2.6+) for opening the file
 (Read/Write/Create/Seek to start) instead of `r+`(Read/Write/Seek to
 start) but `touch()` works too.

 > It would be useful to not write the file at all if it won't change.
 Perhaps it could figure out what the original contents look like, then
 compare the old and new, and skip the write if they're identical.

 With the locking strategy currently in use (see below) this only gives a
 minor speed improvement, but it seems like a good option.

 > I noticed that in this changeset, the fopen call lost the leading @.
 fopen will throw an E_WARNING when the fopen fails, so I expect it might
 be desired to keep it as @fopen.

 I've left it, as the pre-checks should prevent that being the case.

 > I also wonder if it would be best practice to call fflush($fp) before
 flock( $fp, LOCK_UN )
 Certainly can't do any harm, and is probably the best since PHP buffers at
 8K. There's a chance another thread could lock the file after closing the
 lock and closing the file handler (which flushes).

 > And finally, I wonder if it wouldn't be best to wait until right before
 the fseek( $fp, 0 ) to make the blocking flock( $fp, LOCK_EX ) call. Isn't
 it a best practice to hold a blocking lock for the minimum amount of time
 you can?

 This is a tough one.
 Currently it locks the file while it's working to ensure that all writes
 DO make it to the file. Ie. ProcessA and ProcessB's writes will both end
 up in the file (if they're different). But if ProcessC ... Z are also
 trying to write to the file, then it's going to cause a traffic jam.
 If we were to only lock during the write phase, then ProcessA and ProcessB
 if altering different `$marker`s would cause ProcessA's changes to be
 lost.
 Another option would be to use `LOCK_NB` (non-blocking) and detect an
 existing lock, if one is already set then bail from the function entirely
 causing the second writes to fail.

 Personally I'm thinking of leaving it as-is and blocking and waiting,
 because if you're writing on every pageload you're doing bad things.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/31767#comment:22>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform