When my site loads, it usually throws a few warnings and consequently I get sent to line 656 of common.inc, which calls filter_xss as of Drupal 6.21. The trouble is that filter_xss is not defined at that point in the bootstrap sequence, so when I load any page, I get a whitescreen with the error "Fatal error: Call to undefined function filter_xss() in /var/www/dev.treepeople.org/includes/common.inc on line 656".

An easy fix is just to create a filter_xss* function in common.inc, but this seems redundant. Any ideas? If filter_xss* is the best solution, I can submit the patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eric_A’s picture

Priority: Normal » Critical
Eric_A’s picture

Version: 6.21 » 6.x-dev
Status: Active » Needs review
FileSize
541 bytes

This patch takes the approach of simply loading filter.module if filter_xss() is needed.

Eric_A’s picture

This bug is promoting all kinds of errors to fatal errors, including errors that occur during updates to 6.21/6.22...

Eric_A’s picture

Title: filter_xss undefined when called in common.inc » drupal_error_handler() assumes filter_xss() is available before full bootstrap
Component: filter.module » base system

The bug is not in filter.module but in the error handler. That makes the component... base system?

Eric_A’s picture

Wait a minute... under what conditions is drupal_error_handler() set as handler *before* a full bootstrap...?

Eric_A’s picture

Title: drupal_error_handler() assumes filter_xss() is available before full bootstrap » drupal_error_handler() assumes filter_xss() is available before fully bootstrapped

@andywalters: which warnings are you getting?
I'd say the error handler is triggered in _drupal_bootstrap_full() around fix_gpc_magic() *or* some other piece of code is registering the error handler in an earlier phase.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

+1 for #2.

Regarding the question in #5, to give just one example, if some code should happen to output some text prior to a full bootstrap, then the call to drupal_set_header (which is the line immediately following the line that sets the error handler), then a message will be logged and the bootstrap will fail. While this particular error should never happen, it is annoying when it does.

Regarding #6, if the general consensus is, in fact, that none of the code between set_error_handler and module_load_all should ever throw a log message, then the appropriate fix would be to move the call to set_error_handler should be moved below module_load_all in case the supposition ever turns out to be false. However, I think that the fix in #2 is better.

Note also that there is no problem with the placement of filter_xss in Drupal 7.

halka-dupe’s picture

yktdan’s picture

Breaks CiviCRM 3.2.4 install. (and so I get notified when fixed)

greg.1.anderson’s picture

I will also add that this error happens a lot when running drush in --debug mode.

rustybike’s picture

subscribing

mithralsun’s picture

Hi, how do you apply the patch? do I just copy it into the file or do I replace the whole section?

grendzy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
690 bytes

There was a debate within the security team between check_plain and filter_xss. We did not resolve this question, but instead decided it was more important to release a fix promptly and debate later. :-)

To summarize:
The default PHP error handler allows HTML errors, so we should replicate this behavior.
vs
The standard PHP behavior is broken, so we should not replicate it.

Anyway, using check_plain would also resolve the undefined function problem, so here's an alternative patch:

forcesnoir’s picture

@ mithralsun

repalce this:
$entry = check_plain($types[$errno]) .': '. filter_xss($message) .' in '. check_plain($filename) .' on line '. check_plain($line) .'.';

with this:
$entry = check_plain($types[$errno]) .': '. check_plain($message) .' in '. check_plain($filename) .' on line '. check_plain($line) .'.';

// Force display of error messages in update.php.
if (variable_get('error_level', 1) == 1 || strstr($_SERVER['SCRIPT_NAME'], 'update.php')) {

in common.inc

Eric_A’s picture

Status: Needs review » Reviewed & tested by the community

Between #2 and #13 the patch in #13 is the safest fix for D6 and produces the safest output. The only downside of #13 is the difference with the filtering in D7.

Since the D6 issue queue is filling up with people reporting a 6.21/ 6.22 WSOD it would be good to commit this quickly.

Eric_A’s picture

Title: drupal_error_handler() assumes filter_xss() is available before fully bootstrapped » WSOD: drupal_error_handler() assumes filter_xss() is defined
mdlueck’s picture

subscribe... I thought it was just a Drush thing, having it run core-cron. That is the only bad thing I noticed upgrading from D6.20 to D6.22, and had to go back to wget running cron without verbose logging. I sure would like to get it fixed! Please speak up if you need something tested.

Skidz’s picture

Subscribing

pcs305’s picture

FWIW,
I got this error after restoring my 6.22 site to a dev server and forgot to change the database name in settings.php, the userid password was correct but the db did not exist.

RicardoDavis’s picture

Issue tags: +WSOD

I'm using v. 6.16 and created a copy of the site for testing. Used drush to update to 6.22, then got the WSOD. Did a web search to find this article based on the error; applied the patch to includes/common.inc but I'm still getting the WSOD. If I run the site index.php in the PHP CLI I get the following:

#php index.php

Notice: Undefined index: REMOTE_ADDR in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 1317

Notice: Undefined index: REMOTE_ADDR in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 1317

Warning: session_start(): Cannot send session cookie - headers already sent by (output started at /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc:1317) in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 1161

Warning: session_start(): Cannot send session cache limiter - headers already sent (output started at /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc:1317) in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 1161

Notice: Undefined index: REQUEST_METHOD in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 659

Notice: Undefined index: REMOTE_ADDR in /var/www/vhosts//subdomains/test/httpdocs/includes/bootstrap.inc on line 1317

(last notice repeated 17 times)

I noted that even though the PHP CLI didn't choke, running the index.php in the web browser kills the worker process:

# tail /var/log/apache2/error.log
...
[Fri Jul 22 16:01:47 2011] [notice] child pid 13518 exit signal Segmentation fault (11)

Any ideas on what's happening in this case?

grendzy’s picture

RicardoDavis: the segfault is what's causing your wsod - it's not related to this issue. Unfortunately, segfaults are unusual and hard to diagnose. Sometimes it indicates a bug in PHP. Sometimes highly recursive regexes can cause a segfault, you can try deleting contrib modules a few at a time to see if they are triggering it.

mdlueck’s picture

Any progress on this critical issue aside from what is documented here? This thread has been painfully silent.

#13 Posted by grendzy on June 20, 2011 at 6:19pm

... but instead decided it was more important to release a fix promptly and debate later. :-)

I whole hardheartedly agree! So did that mean (since the patch is green) that I should apply the patch prior to receiving the fix via an official Drupal release?

bdone’s picture

subscribing...and confirming that the patch in #13 removes the error in drush output, when running the following command:

drush sql-sync @mysite.staging @mysite.localhost -y --no-cache --sanitize

in my setup, that error didn't begin occurring until a recent core upgrade to 6.22. should this be added as a drush issue as well? if so, i'll be happy to create an issue over there.

mdlueck’s picture

@bdone #23, there are several bugs open against drush hinting at this problem. The Drush developers seem bewildered at this bug, since it really is a core bug I guess I can accept their bewilderment.

mdlueck’s picture

FYI: Patch at #13 is indeed the temp work-around to this issue. Drush is able to run core-cron once again with only that one patch (one LOC updated) applied. Thank you.

bakr’s picture

#13: 1174496.patch queued for re-testing.

Damien Tournoud’s picture

Version: 6.x-dev » 8.x-dev
Priority: Critical » Normal
Issue tags: -WSOD +Needs backport to D6, +Needs backport to D7

Something like #13 needs to happen in 8.x first.

Eric_A’s picture

Priority: Normal » Critical

Something like #13 needs to happen in 8.x first.
What is the motivation for this? The issue at hand does not exist in D8 and D7. The filter_xss() function is included in time in D8 and D7. In D8 and D7 there is no fatal error.

Eric_A’s picture

Priority: Critical » Major

Maybe not critical, but broken 6.x-6.21 update paths, a broken drush and then you have an error handler that promotes notices to fatal errors...

sun’s picture

Status: Reviewed & tested by the community » Needs review

It looks to me this issue did not occur yet in D7+, so I'd kinda agree that it's D6- only.

OTOH, the change to check_plain() itself is concerning with regard to more "rich" messages/errors (as being produced by debug()), so a ternary function_exists() operation would seem most adequate... but then again, that would mean that we no longer know from where and how errors are generated.

grendzy’s picture

I thought Heine might want to explain the case for check_plain, but in the meantime I'll paraphrase: In some circumstances treating an error as HTML loses information. Consider the following code:

error_reporting(E_ALL | E_STRICT);
$foo = array();
if ($foo['<strong>']) {}

If you use check_plain, the meaning (there is no array index named <strong>) of the message is preserved:
• Notice: Undefined index: <strong> in /private/tmp/test.php on line 5

If you use filter_xss, the meaning is lost, and instead you'll see:
• Notice: Undefined index: in /private/tmp/test.php on line 5

It's true the WSOD is D6-only. Damien's point about D8 is that if we switch to check_plain, that's an API change and would need to start in HEAD. Given that we probably don't want to leave D6 users in the lurch for so long, I think sun's idea is the most expedient resolution. Can someone roll a new patch?

greg.1.anderson’s picture

Here is a new patch that mostly follows sun's suggestion. filter_xss is used if it is defined; early in the bootstrap, before filter_xss is available, check_plain is used. This means that some information may be lost from $message if the error handler is called in the early stages of the bootstrap, but this is fairly rare. I think this strikes a good balance between safety, fidelity, and conformance to drush-8.x behavior.

Status: Needs review » Needs work

The last submitted patch, 0682-1174496-by-grendzy-and-greg.1.anderson-call-check_pl.patch, failed testing.

grendzy’s picture

Version: 8.x-dev » 6.x-dev
Status: Needs work » Reviewed & tested by the community

Yep, that's it. I'm changing the version back to D6 because this proposal is a D6 patch, and essentially preserves the existing error handler behavior, so no change is required in D7/8.

grendzy’s picture

bakr’s picture

In principal, this error should not ever appear to anyone, in fact, it should not exist at all.

When I faced that recently, it was due to the following:

* I had memcache enabled as a module (cache.inc + session.inc)
* the memcache deamon was down

Due to this Fatal Error, I was not able to enter drupal in order to clear cache !!!

I have deleted the memcache folder from module directory

then I was back to normal...

All what can I say is that I relate the matter to some sort of stale cache state.

Best Regards

ldweeks’s picture

Status: Reviewed & tested by the community » Needs work

I just installed Drupal 6.22 on MAMP with the patch from #32, and then I installed CiviCRM 3.4.6. I got farther than I did without the patch, but it was still broken. Before the patch, the civicrm module didn't install. After the patch, the civicrm module installed, but civicrm was still completely broken. I tried it with php 5.2.17 and 5.3.6. I'm using mac os x 10.6.8.

grendzy’s picture

Status: Needs work » Reviewed & tested by the community

In the absence of any specific information implicating the error handler in your CiviCRM issue, I'm resetting the status.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Needs review

Regarding #37, this issue is not about "CiviCRM does not work with Drupal 6.22." This issue is about filter_xss in drupal_error_handler. Please do not change the status based on unrelated problems. Maybe you should look for help with CiviCRM at http://forum.civicrm.org/.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Miss-set status; resetting per #38.

greg.1.anderson’s picture

CiviCRM issue tracker is at http://issues.civicrm.org/

ldweeks’s picture

@greg.1.anderson: My apologies. I was in a hurry at the end of the day and realized as I was driving away that I submitted this to the wrong issue queue! Thanks for your work.

webchick’s picture

Issue tags: -Needs backport to D7

It looks like this no longer needs backport to D7, so removing that tag.

sanjaya117’s picture

bixgomez’s picture

What is the status of this issue? I am unable to install CiviCRM 3.4.7 on Drupal 6.22 because of this. Which patch is the solution?

jennypanighetti’s picture

Chris4711’s picture

OK, folks, I am abandoning Drupal and CiviCRM now. This endless discussion about an error that should not exist, full of typos and wrong links, going on for 6 months with full releases in between. It makes it impossible to install CiviCRM and there is not a single word I can understand.

Back to Wordpress !

mdlueck’s picture

@Chris4711 ala #47: So what was "wrong" with my confirmation in #25 ( http://drupal.org/node/1174496#comment-4855476 ) that the patch attached to #13 ( http://drupal.org/node/1174496#comment-4629384 ) works as a workaround to this issue? The patch is still green, indicating it is a good / viable patch. Apply it and be done with the error already! 'tis not rocket science nor brain surgery. (sheesh...)

pjek’s picture

I have just manually patched the code and the problem was solved, just did what the patch in #13 was supposed to do, replaced the "-" line with the "+"line.

Is that dangerous behavior?

jzacsh’s picture

#32 looks awesome sauce to me. Is this going to make it into 6 anytime soon?

I patched my installation myself, but seems like this code can be committed now.

crotown’s picture

c31ck’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
889 bytes

Patch from #32 looks like a good fix for this. Just one remark: perhaps we should consider adding a code comment? It might not be obvious to someone viewing that piece of code why we sometimes use filter_xss() and use check_plain() at other times.

Attached a patch that adds a comment, the functionality remains the same.

klaasvw’s picture

Status: Needs review » Reviewed & tested by the community

Patch is solving the issue for me. Resetting the status so this can make it into 6.

greg.1.anderson’s picture

Issue tags: -Needs backport to D6

@webchick says that there will be a d6 release on 1 February. What would it take to get this committed in time to be included? There hasn't been any negative feedback from the community about this solution yet, and I find it necessary to patch all of my d6 sites with this to avoid problems with Drush.

I have removed the "needs backport to d6" tag, in case that was causing confusion -- this is a d6-only issue that does not occur in d7. Unless someone has an objection to the proposed solution, this should go in now.

greg.1.anderson’s picture

Word is that the D6 RTBC queue will be reviewed and committed prior to the next d6 release, so this issue should go in shortly if no problems are found. Thanks, all who have been working on this.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

greg.1.anderson’s picture

Thank you so much.

aaronortega’s picture

Status: Fixed » Needs review
Heine’s picture

Status: Needs review » Fixed

:|

mdlueck’s picture

Confirmed: Just finished up applying D6.24 on all sites. Works successfully with Drush on both php-cli and php-cgi.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.