WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#7391 closed defect (bug) (fixed)

forbiddance of ini_set('include_path', ...) function causes fatal error at revision feature

Reported by: codestyling Owned by:
Milestone: 2.6.1 Priority: high
Severity: blocker Version: 2.6
Component: General Keywords: has-patch needs-testing
Focuses: Cc:

Description

The newly introduced revision feature uses PHP ini_set() function but must not. Several provider doesn't permit setting php.ini values especially the include_path, which should be temporary switched during load of Text/Diff files. The fatal error resulting looks like this out of real customer occurance:

Warning: ini_set() has been disabled for security reasons in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/pluggable.php on line 1517
Warning: require_once(Text/Diff/Renderer.php) [function.require-once]: failed to open stream: No such file or directory in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/Text/Diff/Renderer/inline.php on line 17
Fatal error: require_once() [function.require]: Failed opening required ‘Text/Diff/Renderer.php’ (include_path=’.:/srv/www/httpd/phost/l/de/pytalhost/laz/web’) in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/Text/Diff/Renderer/inline.php on line 17

This error affects all editor pages won't longer behave as expected. Moreover some provider doesn't show any errors and silently abort the engine. This results in white pages served!
I have attached a patch suggestion (containing 4 file from WP 2.6.0) which doesn't use ini_set() anymore but all require calls now using _ _FILE_ _ evaluation of their appropriated complete file path. Has been successful tested at this affected system. Can also be simulated, if you comment the ini_set() inside pluggable.php function wp_text_diff().

Attachments (6)

wp-includes-WP-2.6.0.zip (21.6 KB) - added by codestyling 12 years ago.
7391.patch (1007 bytes) - added by azaozz 12 years ago.
7391.r8483.diff (1.6 KB) - added by jacobsantos 12 years ago.
Use set_include_path() and restore_include_path() based off of r8483
7391.r8561.diff (2.0 KB) - added by santosj 12 years ago.
Patch for trunk (r8561) sets the include path in wp-settings if Revisions aren't disabled.
7391.r8561.2.6.diff (2.0 KB) - added by santosj 12 years ago.
Set include path in wp-settings for 2.6 r8561
7391.r8579.diff (2.3 KB) - added by ryan 12 years ago.

Download all attachments as: .zip

Change History (25)

#1 @santosj
12 years ago

Could use set_include_path() function instead.

#2 @santosj
12 years ago

Can you create a new file with <?php phpinfo();

and report what disable_functions setting has?

#3 @codestyling
12 years ago

At the moment those functions are disabled:
exec,passthru,system,shell_exec,proc_open,proc_get_status,proc_close,escapeshellcmd,es capeshellarg,ini_set,ini_restore

#4 @azaozz
12 years ago

  • Keywords reporter-feedback added; fatal error crash removed

Perhaps set_include_path() would work here and if not, can show error message instead.

@azaozz
12 years ago

#5 @jacobsantos
12 years ago

You have a typo in the message.

@jacobsantos
12 years ago

Use set_include_path() and restore_include_path() based off of r8483

#6 follow-up: @jacobsantos
12 years ago

Should probably return WP_Error() instead of a string. However, I'm not going to dig down into the wp-admin structure in order to make that work.

#7 in reply to: ↑ 6 @azaozz
12 years ago

Replying to jacobsantos:

Should probably return WP_Error() instead of a string. However, I'm not going to dig down into the wp-admin structure in order to make that work.

Yes, was thinking to return WP_Error and change revision.php so it can handle errors.

Don't think a conditional is needed here. If a host has disabled set_include_path() why would they leave ini_set() enabled. Also what's the purpose of running restore_include_path() at this point? The include path may have been extended before wp_text_diff() was called and restoring it to default may break another function.

#8 @jacobsantos
12 years ago

Good Points. Having a fall back is never a bad thing to have and with most instances, the first conditional will evaluate to true. For edge cases where ini_set() is enabled, but set_include_path() is disabled. It seems unlikely, but available.

I don't like error suppression, because you really don't gain anything from it, the error will still be passed to either the Apache error log, so no benefit from that. Also before PHP 5.2, it is quite the performance hit.

If you want to remove restore_include_path() then it is possible. I had it because it already exists within the function. If it currently works, then there is no harm, but there is no harm in keeping it either.

#9 @santosj
12 years ago

This should be in the wp-settings.php. It should create a constant named WP_INCLUDE_PATH_DISABLED, in which case, the include path appears to be set each time the function is called. If it is called once, then no big deal, if it is called 20 times, then the include path is set and then restored 20 times. This adds overhead to the process. It is better to do it once outside of the function.

#10 follow-up: @codestyling
12 years ago

I don't understand the strict order "not modify affected files". These files original comes from Pear Framework and are just a copy out of to be present at WP. So WP may maintain now these copies as the would be part of the product itself.
In this case, these files can be modified to required the needed other ones by using the correct path an not be dependend of other ini manipulation function potential disabled by hoster too. Doing so, not further errors will occure here.
The alternative would be to define WP needs PHP5 (at some hoster it doesn't run any longer with PHP4, i proofed this!) and the classes will be loaded by autoload feature of PHP. Why working around several better ways fo just keeping this few files as is ?

#11 in reply to: ↑ 10 @santosj
12 years ago

Replying to codestyling:

I don't understand the strict order "not modify affected files".

The reason is that when you go to upgrade, you have to keep making those modifications again, and again. This is not ideal.

#12 @jacobsantos
12 years ago

What is needed for the patch?

@santosj
12 years ago

Patch for trunk (r8561) sets the include path in wp-settings if Revisions aren't disabled.

#13 @santosj
12 years ago

  • Keywords has-patch needs-testing added; reporter-feedback removed

Can you check the new patch and see if it is acceptable?

@santosj
12 years ago

Set include path in wp-settings for 2.6 r8561

#14 @santosj
12 years ago

Patch for 2.6 also.

#15 @azaozz
12 years ago

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

Fixed in [8562] and [8563].

#16 @azaozz
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reverted... Patch throws a fatal error.

@ryan
12 years ago

#17 @ryan
12 years ago

7391.r8579: codestyling's changes as a diff.

#18 @ryan
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [8581]) Fix Text/Diff includes. Props codestyling. fixes #7391 for trunk

#19 @ryan
12 years ago

(In [8582]) Fix Text/Diff includes. Props codestyling. fixes #7391 for 2.6

Note: See TracTickets for help on using tickets.