Opened 15 years ago
Last modified 5 years ago
#11903 accepted defect (bug)
insert_with_markers is not threadsafe
Reported by: | strings28 | Owned by: | westi |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | major | Version: | 2.9 |
Component: | Permalinks | Keywords: | has-patch needs-refresh |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (27)
#1
@
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.
#2
@
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
@
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.
#5
@
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
@
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.
#8
@
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
@
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
@
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
@
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
@
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
@
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.
#15
@
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
@
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
#18
@
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
@
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.
#22
@
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.
A proposed patch which creates a lock file