Make WordPress Core

Opened 15 years ago

Last modified 5 years ago

#11903 accepted defect (bug)

insert_with_markers is not threadsafe

Reported by: strings28's profile strings28 Owned by: westi's profile westi
Milestone: Priority: normal
Severity: major Version: 2.9
Component: Permalinks Keywords: has-patch needs-refresh
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 (4)

misc-with-patch.php (17.5 KB) - added by strings28 15 years ago.
A proposed patch which creates a lock file
11903-by-strings28.diff (1.3 KB) - added by miqrogroove 15 years ago.
Diff proper for strings28
11903.wp-admin.includes.misc.php.diff (3.9 KB) - added by loushou 10 years ago.
patch for the mod_rewrite config to only write the file when needed
11903.wp-admin.includes.misc.php.2.diff (4.2 KB) - added by loushou 10 years ago.
THE CORRECT PATCH. the first one was an earlier, bad version i had

Download all attachments as: .zip

Change History (27)

@strings28
15 years ago

A proposed patch which creates a lock file

#1 @strings28
15 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.

@miqrogroove
15 years ago

Diff proper for strings28

#2 @miqrogroove
15 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.

#3 @Denis-de-Bernardy
15 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.

#4 @Denis-de-Bernardy
15 years ago

  • Description modified (diff)

#5 @miqrogroove
15 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?

#6 @strings28
15 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.

#7 @strings28
15 years ago

  • Cc strings28 added

#8 @Denis-de-Bernardy
15 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;
}

#9 @Denis-de-Bernardy
15 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.

#10 @miqrogroove
15 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?

#11 @miqrogroove
15 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.

#12 @miqrogroove
15 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.

#13 @miqrogroove
15 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.

#14 @miqrogroove
15 years ago

See related ticket #12324

#15 @miqrogroove
15 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.

#16 @miqrogroove
15 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

#17 @miqrogroove
15 years ago

$wp_rewrite->flush_rules(FALSE);

#18 @miqrogroove
15 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:

#19 @westi
14 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.

#20 @nacin
14 years ago

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

#21 @retlehs
12 years ago

  • Cc retlehs added

#22 @loushou
10 years ago

  • Keywords has-patch added; needs-patch removed

After reading this whole thing, I totally agree that the right approach is to only write the config files when they actually have changes that need writing, instead of some other approach of limiting writing. A little digging shows that the IIS write functions already seem to do this. The mod_rewrite config writing function however does not seem to cover this yet.

The function already checks if the block exists, but it does not make any distinction of whether it is the same block or not, and just blindly overwrites it. I adapted the function to test if the found block is the same block we are writing. If it is, we just don't write. If it is not the same block, then we replace it. Also if the file does not exist, or the block does not exist already, we still add it to the end of the file.

Seems to work on 4.0+ (and the current nightly). Discuss.

@loushou
10 years ago

patch for the mod_rewrite config to only write the file when needed

@loushou
10 years ago

THE CORRECT PATCH. the first one was an earlier, bad version i had

#23 @chriscct7
9 years ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.