WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10011 closed defect (bug) (wontfix)

Use of add_theme_page() causes Wordpress on IIS to crash.

Reported by: bforchhammer Owned by:
Milestone: 2.9 Priority: normal
Severity: major Version: 2.7.1
Component: Plugins Keywords: needs-patch early close
Focuses: Cc:

Description

The P2 Theme uses the following line to add a theme settings page:

add_theme_page('Prologue Options', 'Prologue Options', 8, __FILE__, 'prologue_options_page');

On IIS this causes a 500 Error and the admin section can not be loaded anymore.

When we substitute __FILE__ by a string, e.g. "p2" everything starts behaving again.

The following might be the cause of the problem:

__FILE__ is passed through the function plugin_basename() in add_submenu_page()... usually that function strips out the path and returns only the plugin filename; for themes that does not work and the function returns the full path to the theme's functions.php.

I don't know why it only crashes on IIS but the value of plugin_basename() is used for a few things including for registering an action hook (which means that it's probably used as a key of an array at some point).

Attachments (2)

10011.diff (645 bytes) - added by bforchhammer 5 years ago.
10011.theme.patch (672 bytes) - added by bforchhammer 5 years ago.
Hm, theme_basename() doesn't look right to me.. $theme_dir is set up but not actually used in the preg_replace() call…

Download all attachments as: .zip

Change History (42)

comment:1 follow-up: Denis-de-Bernardy5 years ago

  • Component changed from General to Plugins
  • Keywords needs-patch added
  • Milestone changed from Unassigned to 2.8
  • Severity changed from normal to major

what does plugin_basename() return?

comment:2 follow-up: peaceablewhale5 years ago

I have installed the P2 theme (http://wordpress.org/extend/themes/p2) to the latest trunk which runs on IIS 7.5 and PHP 5.3.0 RC2 through FastCGI. No error, warning or notice is displayed. The 'Prologue Options' page works successfully. Which version of IIS and PHP are you using?

comment:3 in reply to: ↑ 1 bforchhammer5 years ago

Replying to Denis-de-Bernardy:

what does plugin_basename() return?

The full path to the theme's functions.php...
E:/xampp/wordpress/wp-content/themes/mytheme/functions.php

comment:4 in reply to: ↑ 2 bforchhammer5 years ago

Replying to peaceablewhale:

I have installed the P2 theme (http://wordpress.org/extend/themes/p2) to the latest trunk which runs on IIS 7.5 and PHP 5.3.0 RC2 through FastCGI. No error, warning or notice is displayed. The 'Prologue Options' page works successfully. Which version of IIS and PHP are you using?

Microsoft-IIS/7.0 with PHP Version 5.2.5 (x64) though FastCGI as well.

comment:5 follow-up: peaceablewhale5 years ago

Thanks for the info. Would you add <httpErrors existingResponse="PassThrough" /> to the "system.webServer" section of your web.config file and post the 500 error message here? Please remove the the PassThrough statement after that.

comment:6 in reply to: ↑ 5 bforchhammer5 years ago

Replying to peaceablewhale:

Thanks for the info. Would you add <httpErrors existingResponse="PassThrough" /> to the "system.webServer" section of your web.config file and post the 500 error message here? Please remove the the PassThrough statement after that.

Adding <httpErrors existingResponse="PassThrough" /> does not change anything.

All it says is "The page cannot be displayed because an internal server error has occurred.".

Hm, I should add that he spins for 30s and then times out. 30s is what's set as max_execution_time.

comment:7 peaceablewhale5 years ago

Still can't reproduce the issue with PHP 5.2.9-2 NTS...

The Prologue Options page is loaded successfully and the URL of the page is "WORDPRESS-URL/wp-admin/themes.php?page=C:/wwwroot/wordpress/wp-content/themes/p2/functions.php".

So, "E:/xampp/wordpress/wp-content/themes/mytheme/functions.php" seems to be the intended path.

comment:8 dd325 years ago

have you looked in t he logs yet?

It'd not surprise me if you've got a security function installed which detects ?xxxx=c:/... as remote file inclusion exploitation attempt..

comment:9 Denis-de-Bernardy5 years ago

could also be an infinite loop. or the seems_utf8 issue that was highlighted in a ticket from a day or two ago.

comment:10 bforchhammer5 years ago

Okay, I've tracked it down into menu-header.php and a file_exists() call.

file_exists("E:\Inetpub_Prod\wwwroot/wp-content/plugins/E:/Inetpub_Prod/wwwroot/wp-content/themes/p2/functions.php");

The path is a combination of WP_PLUGIN_DIR and that __FILE__ variable passed to add_theme_page; it obviously is quite wrong and file_exists() should simply return false.

In our environment however it spins and times out... very strange.

I've copied the code above into an empty php file and played around with it a bit; it seems to timeout as soon as there's a second e:/ in the path; wrong slashes, wrong path, etc. doesn't cause a timeout.

Could you guys try this on your IIS boxes and check how your server reacts?

It might just be a wrong server setting or php.ini configuration, or maybe a PHP/IIS Bug, but maybe WordPress should also do some cleaning up on that filename?

What do you guys think?

comment:11 follow-up: ryan5 years ago

WP_PLUGIN_DIR should not contain a URL. Do you hard code it to something, or let wp-settings.php set it up?

comment:12 ryan5 years ago

Oops, duh, that's a windows path, not a url. Silly me, can't read.

comment:13 follow-up: peaceablewhale5 years ago

Seem to be caused by this PHP bug: http://bugs.php.net/bug.php?id=44412

I cannot reproduce the error in both PHP 5.2.9-2 NTS and PHP 5.3.0RC2 NTS. Would you update your PHP?

comment:14 Denis-de-Bernardy5 years ago

haven't checked or tested, but this:

file_exists("E:\Inetpub_Prod\wwwroot/wp-content/plugins/E:/Inetpub_Prod/wwwroot/wp-content/themes/p2/functions.php");

seems like an an issue in the plugin_folder() function et al. or their mis-use in p2.

comment:15 in reply to: ↑ 11 bforchhammer5 years ago

Replying to ryan:

WP_PLUGIN_DIR should not contain a URL. Do you hard code it to something, or let wp-settings.php set it up?

I let wp-settings.php do the work...

comment:16 in reply to: ↑ 13 ; follow-up: bforchhammer5 years ago

Replying to peaceablewhale:

Seem to be caused by this PHP bug: http://bugs.php.net/bug.php?id=44412

I cannot reproduce the error in both PHP 5.2.9-2 NTS and PHP 5.3.0RC2 NTS. Would you update your PHP?

5.2.5 seems to be the latest available 64bit version; I can ask our server guy but I don't think he's going to download and compile a newer version himself...

comment:17 bforchhammer5 years ago

add_submenu_page( $parent, $page_title, $menu_title, $access_level, $file, $function = '' )

If $file is set to __FILE__ in a plugin then the $file parameter gets cleaned up, and the absolute path to the file converted to a relative one (relative to WP_PLUGIN_DIR).

If the same thing is done in a theme's functions.php then the absolute path is not converted to a relative path!

I think the example in Codex does not recommend the use of __FILE__ anymore but there are probably still many themes out there using it.

bforchhammer5 years ago

comment:18 bforchhammer5 years ago

That patch should take care of the error... (needs testing; we're still on 2.7.1 but theme_basename() was added in 2.8).

comment:19 follow-up: dd325 years ago

My only negitive point to the patch: Not only themes add items under there.

Perhaps a helper function could be used:

function the_base($in) {
$plugin = plugin_basename($in);
if ( $plugin != $in )
return $in;
else
return theme_basename($in); //No need to check this, a non-theme dir passes right through.
}

and use that for all menu's.

comment:20 dd325 years ago

of course, that first return should return $plugin..

comment:21 Denis-de-Bernardy5 years ago

-1 on the patch too. This ought be handled further downstream, and for all menu types if it's needed.

comment:22 in reply to: ↑ 16 ; follow-up: peaceablewhale5 years ago

Replying to bforchhammer:

5.2.5 seems to be the latest available 64bit version; I can ask our server guy but I don't think he's going to download and compile a newer version himself...

x64 versions are not officially supported and they usually run slower than x86 versions.

comment:23 in reply to: ↑ 22 bforchhammer5 years ago

Replying to peaceablewhale:

x64 versions are not officially supported and they usually run slower than x86 versions.

Hm, good to know... I'll forward that note to our server admin ;-)

bforchhammer5 years ago

Hm, theme_basename() doesn't look right to me.. $theme_dir is set up but not actually used in the preg_replace() call...

comment:24 in reply to: ↑ 19 bforchhammer5 years ago

Replying to dd32:

My only negitive point to the patch: Not only themes add items under there.

Perhaps a helper function could be used:

function the_base($in) {
$plugin = plugin_basename($in);
if ( $plugin != $in )
return $in;
else
return theme_basename($in); //No need to check this, a non-theme dir passes right through.
}

and use that for all menu's.

I am not sure we need a separate function to check because as far as I can see non-plugin dirs pass right through plugin_basename() as well don't they? It should be possible to just use them after each other.

$file = plugin_basename($file);
$file = theme_basename($file);

Of course if someone would put a completely different path in it would fail again and maybe a more general approach would be better?

Hm, other functions (menu-header.php) still assume that $file is relative to the plugins directory so that probably would need to be changed as well.

comment:25 follow-up: dd325 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

You're right... that would work just as well.

i'm not too sure it matters about other parts of WordPress expecting those parts to be relative to the plugins directory, The chances of a plugin of the same name with a file called 'functions.php' is incredibly unlikely. Furthurmore, I think that the hookname would still be reconised correctly.

So the solution here, seems to be to replace plugin_basename($file) with theme_basename(plugin_basename($file)) in all of the add-menu calls IMO.

Mind you, Thats going to require some testing(And given 2.8's nearing release), I'd suggest that this gets delayed to early 2.9 to allow for some proper testing?

If someone thinks of a better way thats absolutely 100% backwards compatible.. then speak up..

comment:26 in reply to: ↑ 25 bforchhammer5 years ago

Replying to dd32:

So the solution here, seems to be to replace plugin_basename($file) with theme_basename(plugin_basename($file)) in all of the add-menu calls IMO.

Sounds good to me.

That would probably not change anything in the current behaviour; as far as I can see the full path is not used for anything else but the "action hook name" at the moment so it shouldn't break anything... (?)

If I read the code correctly it should probably also be possible to point the path to a different file instead of using a function callback for the menu item; the last parameter in add_submenu_page() seems optional.

Mind you, Thats going to require some testing(And given 2.8's nearing release), I'd suggest that this gets delayed to early 2.9 to allow for some proper testing?

Fine by me :)

comment:28 westi5 years ago

  • Cc westi added

comment:30 follow-up: Denis-de-Bernardy5 years ago

#10132 has a patch that needs testing

comment:31 in reply to: ↑ 30 bforchhammer5 years ago

Replying to Denis-de-Bernardy:

#10132 has a patch that needs testing

#10132 does not address the issue with the php bug mentioned earlier (http://bugs.php.net/bug.php?id=44412). In other words, it is still possible that the file_exists call receives an invalid path (see Comment 10 or am I missing something? Correct me if I'm wrong... :)

comment:32 Denis-de-Bernardy5 years ago

we understood it the same way. the only thing is the php bug is about a warning, whereas we're getting a fatal.

comment:33 Denis-de-Bernardy5 years ago

oh, sorry, I was thinking you mentioned the bug I had referenced in #10132. :-)

comment:34 follow-up: peaceablewhale5 years ago

I have confirmed that http://bugs.php.net/bug.php?id=44412 exists in PHP 5.2.5 with open_basedir set, the PHP script will not return anything until it times out.

The bug has been fixed since PHP 5.2.6. With open_basedir set, a PHP warning is returned: "PHP Warning: file_exists() [function.file-exists]: open_basedir restriction in effect.".

Using @file_exists() will get "false".

Is it really worth working around the PHP bug?

comment:35 in reply to: ↑ 34 bforchhammer5 years ago

Replying to peaceablewhale:

Is it really worth working around the PHP bug?

Considering that version 5.2.5 is the highest available 64bit version I think it is.

comment:36 peaceablewhale5 years ago

However, that build is not officially supported by PHP and contains known security issues.

comment:37 peaceablewhale5 years ago

  • Keywords close added

The official, experimental x64 versions of PHP 5.3.0 has been released on windows.php.net: http://windows.php.net/qa/

The problem reported in this report should not affect PHP 5.3.0. As a new x64 PHP package has been released, I suggest closing this report.

comment:38 bforchhammer5 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Fine by me.

comment:39 mijk3 years ago

  • Version changed from 2.7.1 to 2.7

This works for me:

  //add_theme_page('GTD Options', 'GTD Options', 8, __FILE__, 'prologue_options_page');
    add_theme_page('GTD Options', 'GTD Options', 'edit_theme_options', 'p2-options-page', 'prologue_options_page');

comment:40 mijk3 years ago

  • Version changed from 2.7 to 2.7.1
Note: See TracTickets for help on using tickets.