[wp-trac] [WordPress Trac] #31767: insert_with_markers() is not atomic, leading to corrupted .htaccess updates under race conditions
WordPress Trac
noreply at wordpress.org
Mon Oct 19 00:48:07 UTC 2015
#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
More information about the wp-trac
mailing list