Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#52 | 1174496-check-plain-if-filter-xss-unavailable-52.patch | 889 bytes | c31ck |
#32 | 0682-1174496-by-grendzy-and-greg.1.anderson-call-check_pl.patch | 1.14 KB | greg.1.anderson |
#13 | 1174496.patch | 690 bytes | grendzy |
#2 | filter_xss-undefined-1174496-2.patch | 541 bytes | Eric_A |
Comments
Comment #1
Eric_A CreditAttribution: Eric_A commentedComment #2
Eric_A CreditAttribution: Eric_A commentedThis patch takes the approach of simply loading filter.module if filter_xss() is needed.
Comment #3
Eric_A CreditAttribution: Eric_A commentedThis bug is promoting all kinds of errors to fatal errors, including errors that occur during updates to 6.21/6.22...
Comment #4
Eric_A CreditAttribution: Eric_A commentedThe bug is not in filter.module but in the error handler. That makes the component... base system?
Comment #5
Eric_A CreditAttribution: Eric_A commentedWait a minute... under what conditions is drupal_error_handler() set as handler *before* a full bootstrap...?
Comment #6
Eric_A CreditAttribution: Eric_A commented@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.
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson commented+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
andmodule_load_all
should ever throw a log message, then the appropriate fix would be to move the call toset_error_handler
should be moved belowmodule_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.
Comment #8
halka-dupe CreditAttribution: halka-dupe commented#2: filter_xss-undefined-1174496-2.patch queued for re-testing.
Comment #9
yktdan CreditAttribution: yktdan commentedBreaks CiviCRM 3.2.4 install. (and so I get notified when fixed)
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson commentedI will also add that this error happens a lot when running drush in --debug mode.
Comment #11
rustybike CreditAttribution: rustybike commentedsubscribing
Comment #12
mithralsun CreditAttribution: mithralsun commentedHi, how do you apply the patch? do I just copy it into the file or do I replace the whole section?
Comment #13
grendzy CreditAttribution: grendzy commentedThere 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:
Comment #14
forcesnoir CreditAttribution: forcesnoir commented@ 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
Comment #15
Eric_A CreditAttribution: Eric_A commentedBetween #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.
Comment #16
Eric_A CreditAttribution: Eric_A commentedComment #17
mdlueck CreditAttribution: mdlueck commentedsubscribe... 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.
Comment #18
Skidz CreditAttribution: Skidz commentedSubscribing
Comment #19
pcs305 CreditAttribution: pcs305 commentedFWIW,
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.
Comment #20
RicardoDavis CreditAttribution: RicardoDavis commentedI'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?
Comment #21
grendzy CreditAttribution: grendzy commentedRicardoDavis: 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.
Comment #22
mdlueck CreditAttribution: mdlueck commentedAny progress on this critical issue aside from what is documented here? This thread has been painfully silent.
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?
Comment #23
bdone CreditAttribution: bdone commentedsubscribing...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.
Comment #24
mdlueck CreditAttribution: mdlueck commented@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.
Comment #25
mdlueck CreditAttribution: mdlueck commentedFYI: 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.
Comment #26
bakr CreditAttribution: bakr commented#13: 1174496.patch queued for re-testing.
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedSomething like #13 needs to happen in 8.x first.
Comment #28
Eric_A CreditAttribution: Eric_A commentedSomething 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.
Comment #29
Eric_A CreditAttribution: Eric_A commentedMaybe 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...
Comment #30
sunIt 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.
Comment #31
grendzy CreditAttribution: grendzy commentedI 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:
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?
Comment #32
greg.1.anderson CreditAttribution: greg.1.anderson commentedHere 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.
Comment #34
grendzy CreditAttribution: grendzy commentedYep, 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.
Comment #35
grendzy CreditAttribution: grendzy commented#32: 0682-1174496-by-grendzy-and-greg.1.anderson-call-check_pl.patch queued for re-testing.
Comment #36
bakr CreditAttribution: bakr commentedIn 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
Comment #37
ldweeks CreditAttribution: ldweeks commentedI 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.
Comment #38
grendzy CreditAttribution: grendzy commentedIn the absence of any specific information implicating the error handler in your CiviCRM issue, I'm resetting the status.
Comment #39
greg.1.anderson CreditAttribution: greg.1.anderson commentedRegarding #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/.
Comment #40
greg.1.anderson CreditAttribution: greg.1.anderson commentedMiss-set status; resetting per #38.
Comment #41
greg.1.anderson CreditAttribution: greg.1.anderson commentedCiviCRM issue tracker is at http://issues.civicrm.org/
Comment #42
ldweeks CreditAttribution: ldweeks commented@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.
Comment #43
webchickIt looks like this no longer needs backport to D7, so removing that tag.
Comment #44
sanjaya117 CreditAttribution: sanjaya117 commented#2: filter_xss-undefined-1174496-2.patch queued for re-testing.
Comment #45
bixgomez CreditAttribution: bixgomez commentedWhat 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?
Comment #46
jennypanighetti CreditAttribution: jennypanighetti commented#32: 0682-1174496-by-grendzy-and-greg.1.anderson-call-check_pl.patch queued for re-testing.
Comment #47
Chris4711 CreditAttribution: Chris4711 commentedOK, 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 !
Comment #48
mdlueck CreditAttribution: mdlueck commented@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...)
Comment #49
pjek CreditAttribution: pjek commentedI 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?
Comment #50
jzacsh CreditAttribution: jzacsh commented#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.
Comment #51
crotown CreditAttribution: crotown commented#32: 0682-1174496-by-grendzy-and-greg.1.anderson-call-check_pl.patch queued for re-testing.
Comment #52
c31ck CreditAttribution: c31ck commentedPatch 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.
Comment #53
klaasvw CreditAttribution: klaasvw commentedPatch is solving the issue for me. Resetting the status so this can make it into 6.
Comment #54
greg.1.anderson CreditAttribution: greg.1.anderson commented@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.
Comment #55
greg.1.anderson CreditAttribution: greg.1.anderson commentedWord 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.
Comment #56
Gábor HojtsyThanks, committed, pushed.
Comment #57
greg.1.anderson CreditAttribution: greg.1.anderson commentedThank you so much.
Comment #58
aaronortega CreditAttribution: aaronortega commented#52: 1174496-check-plain-if-filter-xss-unavailable-52.patch queued for re-testing.
Comment #59
Heine CreditAttribution: Heine commented:|
Comment #60
mdlueck CreditAttribution: mdlueck commentedConfirmed: Just finished up applying D6.24 on all sites. Works successfully with Drush on both php-cli and php-cgi.