WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 15 months ago

#11903 accepted defect (bug)

insert_with_markers is not threadsafe

Reported by: strings28 Owned by: westi
Milestone: Future Release Priority: normal
Severity: major Version: 2.9
Component: Permalinks Keywords: needs-patch
Focuses: Cc:

Description (last modified by Denis-de-Bernardy)

From wp-admin/includes/misc.php the function insert_with_markers may be called multiple times on a busy server and if the htaccess is already in the process of being written it is possible that two PHP threads could attempt to write to it causing corruption such as the following:

# BEGIN WordPress
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteBase /
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /index.php [L]
</IfModule>

# END WordPress
s

Notice the dangling 's' at the last line

Attachments (2)

misc-with-patch.php (17.5 KB) - added by strings28 4 years ago.
A proposed patch which creates a lock file
11903-by-strings28.diff (1.3 KB) - added by miqrogroove 4 years ago.
Diff proper for strings28

Download all attachments as: .zip

Change History (23)

strings284 years ago

A proposed patch which creates a lock file

comment:1 strings284 years ago

As an added note the patch file is what I have used on one server that was seeing a corrupted .htaccess file (and thus a 500 error) multiple times a week and it didn't matter which PHP configuration we setup our bluehost account with, the corruption took place. By using lock files it returns early - I also setup an adaptation of the mark string, which could possibly be adjusted to use the wp-prefix variable so as to help make it more obvious to those looking at the file that it is being inserted by a unique Wordpress installation.

miqrogroove4 years ago

Diff proper for strings28

comment:2 miqrogroove4 years ago

  • Keywords needs-patch added; .htaccess threads removed
  • Milestone changed from Unassigned to 3.0

I like this bug. I don't like the patch for two reasons: It doesn't prevent fopen() races. It doesn't handle error recovery.

I think the correct way to handle this is with flock() and no extra file.

comment:3 Denis-de-Bernardy4 years ago

I don't like the flock() idea. flock() isn't always available and is tricky to use on some servers, as was revealed by plugins such as wp-cache and its derivatives.

the "standard" workaround is to write to a temporary file, and then to rename the temporary file, overwriting the target file as needed.

comment:4 Denis-de-Bernardy4 years ago

  • Description modified (diff)

comment:5 miqrogroove4 years ago

  • Keywords reporter-feedback added; needs-patch removed

Denis and I were discussing this privately. It occurred to me that WordPress isn't supposed to have htaccess races in the first place. That's a static file, and seeing it get updated by WordPress more than once in a month would be unusual. So before we proceed, I think the next step is for strings28 to determine why the htaccess file is being written to at all?

comment:6 strings284 years ago

I would *LOVE* to know why the writing is happening more than once. Since implementing this lock file method it has stopped getting corrupted.

I have searched Google, Bing and the WordPress forums and found no real solid information as to why this would be happening and the server logs did not reveal anything and no PHP errors were reported to have caused this. There were several WordPress installations on our server and I modified the insert_with_marker function to put a customer marker string in for each installation to make sure there wasn't a strange situation where multiple installations were setup incorrectly (and that doesn't appear to be the case).

I also checked the WordPress 'cron' jobs and none of those were doing anything that would be related to this problem. If anyone has any debugging ideas I would LOVE to hear them and attempt to figure out what is going on. Are there functions I should add logging code to to help find this? I'm willing to go the extra mile here, but I'm at a loss where to start.

comment:7 strings284 years ago

  • Cc strings28 added

comment:8 Denis-de-Bernardy4 years ago

you could temporarily add a backtrace in the insert_with_markers() function.

$log = get_option('rewrite_used_log', array());
$log[] = debug_backtrace();
update_option('rewrite_used_log', $log);

and then in wp-config.php, after WP is loaded:

if ( isset($_GET['test']) && current_user_can('manage_options') ) {
header('Content-Type: text/plain; Charset: UTF-8');
var_dump(get_option('rewrite_used_log'));
die;
}

comment:9 Denis-de-Bernardy4 years ago

don't forget to delete the option at some point, too. if it's really getting regenerated on every page load, it can huge very quickly.

comment:10 miqrogroove4 years ago

Denis, I noticed something really weird happen just now. I was editing my .htaccess file, and because I'm too lazy to do the temporary file thing, I simply FTP'd it over the existing file. For some reason this caused WordPress to flush the interal rewrite rules. Normally I wouldn't notice something like that, but I had a bug in one of my rewrite hooks, and this took down the whole website until I fixed my plugin.

Any idea what the connection is between .htaccess changes and spontaneous rule flushing?

comment:11 miqrogroove4 years ago

Looks like WordPress will do a rule flush any time a page is deleted. I haven't found anything that would cause it to happen spontaneously due to .htaccess changes.

The temporary file idea is a good one. Not all servers have a temporary directory set up correctly, but that could be tested to fall back on direct writes.

comment:12 miqrogroove4 years ago

flush_rules is also indirectly hooked for saving page type posts. In the situation I mentioned above, there's a good chance I had edited a page right before editing the .htaccess file, and it was just a coincidence.

comment:13 miqrogroove4 years ago

Another important thing that needs to happen in insert_with_markers() is to NOT write to the file if the output is identical to the input.

comment:14 miqrogroove4 years ago

See related ticket #12324

comment:15 miqrogroove4 years ago

The logic flaw described by the other ticket is caused by the insert function running out of control when any duplicate marker exists within .htaccess. The insert function needs to return unconditionally if that condition is ever encountered.

comment:16 miqrogroove4 years ago

We also have a documentation problem where rewrite(FALSE) is not explicitly shown in code snippets:

http://codex.wordpress.org/Function_Reference/WP_Rewrite
and
http://codex.wordpress.org/Custom_Queries#Permalinks_for_Custom_Archives

comment:17 miqrogroove4 years ago

$wp_rewrite->flush_rules(FALSE);

comment:18 miqrogroove4 years ago

  • Keywords needs-patch added; reporter-feedback removed

Another bug to throw on the pile here:

GET /wp-admin/options-permalink.php

... causes a flush every time D:

comment:19 westi4 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1
  • Owner changed from ryan to westi
  • Status changed from new to accepted

I don't think that adding locking around the file write is the right thing to do here.

The correct solution is to only write the file changes when necessary - which considering the content of the rules we write is not very often.

Moving out of 3.0 for now as we don't have a suitable patch and we are close release.

comment:20 nacin3 years ago

  • Keywords early removed
  • Milestone changed from Awaiting Triage to Future Release

comment:21 retlehs15 months ago

  • Cc retlehs added
Note: See TracTickets for help on using tickets.