Optimize check_plain()

Hm. drupal_validate_utf8() additionally checked for empty()... why not here?

Also, even after some discussion in IRC, I'm still not sure whether we can't entirely drop that IE6 support function. See also #308865: Drop IE6 support in Drupal core.

Additional insight for why this function exists: https://security.drupal.org/node/116

I'd propose to re-think whether we need this entire check at all. (See also the issue I linked to in my previous follow-up)

empty() will return TURE for the string '0' as well as various non-strings. Are you sure you want to use that vs. the strlen==0 check?

You could also cast to a string and compare to the empty string, and return the cast variable.

@catch- makes sense to me - if there are that few we are not gaining anything since it costs lots of extra function calls.

We should append a @see drupal_validate_utf8() to the PHPDoc though, and perhaps additionally explain why we are duplicating code.

Speaking of duplication, shouldn't we apply the same to filter_xss() (the second instance) ? So we'd keep drupal_validate_utf8() for (rather special) contrib modules that need it only?

Yay! Thanks for updating!

+ *   valid UTF-8. @see drupal_validate_utf8().

(multiple occurrences) @see should always be on a separate line.

 function check_plain($text) {
-  return drupal_validate_utf8($text) ? htmlspecialchars($text, ENT_QUOTES) : '';
+  // Validate strings as UTF-8 to prevent cross site scripting attacks on
+  // Internet Explorer 6. We duplicate the same preg_match() here to avoid
+  // additional function calls, since check_plain() is often called hundreds of
+  // times per request.
+  return (preg_match('/^./us', $text) == 1) ? htmlspecialchars($text, ENT_QUOTES) : '';

This lengthy comment should rather go into the PHPDoc.

Looks good to me - nice to have to validated performance improvements.

The only problem with removing the strlen from drupal_validate_utf8 is that it changes the result for empty string. An empty string is a valid utf8 string, whereas the regex requires at least one character.

strlen() should be lightening fast. What about benchmarking with and without that check?

This was already RTBC. It duplicates a small amount of code (referencing the original) in a critical path which reduces page load by a noticeable amount. Sounds good to me.

+++ includes/bootstrap.inc	18 Jul 2009 19:54:35 -0000
@@ -1024,11 +1024,21 @@ function drupal_unpack($obj, $field = 'd
+ * check_plain() also validates strings as UTF-8 to prevent cross site scripting

...slightly exceeds 80 chars.

Beer-o-mania starts in 22 days! Don't drink and patch.

In PHP 5.2.5 and newer, htmlspecialchars() automatically handles partial multibyte sequences (according to the release notes).

The following approach reduces the running time of check_plain() even further (approx. 11% AFAICT) when using a PHP 5.2.5 or later. With older versions, it is about as fast as than the current implementation.

function check_plain($text) {
  static $new_php;
  if (!isset($new_php)) {
    $new_php = version_compare(PHP_VERSION, '5.2.5', '>=');
  }
  return ($new_php || preg_match('/^./us', $text) == 1) ? htmlspecialchars($text, ENT_QUOTES, 'UTF-8') : '';
}

Let's prove this.

NOTE: As of now, this is NOT the patch that is RTBC.

Complete removal of check_plain(): (sorry, my machine is slow)

HEAD without patch:

# ab -c 2 -n 100 http://drupal.test/

Document Path:          /
Document Length:        10865 bytes

Concurrency Level:      2
Time taken for tests:   38.092 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1128800 bytes
HTML transferred:       1086500 bytes
Requests per second:    2.63 [#/sec] (mean)
Time per request:       761.836 [ms] (mean)
Time per request:       380.918 [ms] (mean, across all concurrent requests)
Transfer rate:          28.94 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   13  11.7     16      62
Processing:   547  744 198.3    625    1641
Waiting:      531  728 197.8    609    1625
Total:        562  757 201.3    656    1656

Percentage of the requests served within a certain time (ms)
  50%    656
  66%    828
  75%    844
  80%    844
  90%    937
  95%   1281
  98%   1422
  99%   1656
 100%   1656 (longest request)

Patch applied:

# ab -c 2 -n 100 http://drupal.test/

Document Path:          /
Document Length:        10865 bytes

Concurrency Level:      2
Time taken for tests:   35.529 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1128800 bytes
HTML transferred:       1086500 bytes
Requests per second:    2.81 [#/sec] (mean)
Time per request:       710.589 [ms] (mean)
Time per request:       355.294 [ms] (mean, across all concurrent requests)
Transfer rate:          31.03 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   13   9.3     16      31
Processing:   547  694 146.5    687    1484
Waiting:      531  678 146.5    672    1469
Total:        562  707 145.7    687    1500

Percentage of the requests served within a certain time (ms)
  50%    687
  66%    750
  75%    766
  80%    766
  90%    859
  95%    937
  98%   1344
  99%   1500
 100%   1500 (longest request)

EDIT: I mistakenly copied the benchmarks in the wrong order :-/

And of course, here is the updated patch.

Using a concurrency of 1:

HEAD:

# ab -c1 -n 100 http://drupal.test/

Document Path:          /
Document Length:        10865 bytes

Concurrency Level:      1
Time taken for tests:   49.904 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1128800 bytes
HTML transferred:       1086500 bytes
Requests per second:    2.00 [#/sec] (mean)
Time per request:       499.037 [ms] (mean)
Time per request:       499.037 [ms] (mean, across all concurrent requests)
Transfer rate:          22.09 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   10   7.7     16      16
Processing:   453  487  48.1    469     734
Waiting:      437  471  48.4    453     719
Total:        469  497  48.5    484     734

Percentage of the requests served within a certain time (ms)
  50%    484
  66%    484
  75%    500
  80%    500
  90%    562
  95%    609
  98%    687
  99%    734
 100%    734 (longest request)

Patch:

Document Path:          /
Document Length:        10865 bytes

Concurrency Level:      1
Time taken for tests:   48.169 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1128800 bytes
HTML transferred:       1086500 bytes
Requests per second:    2.08 [#/sec] (mean)
Time per request:       481.694 [ms] (mean)
Time per request:       481.694 [ms] (mean, across all concurrent requests)
Transfer rate:          22.88 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   10   7.7     16      31
Processing:   453  470  41.3    469     766
Waiting:      437  454  41.4    437     750
Total:        453  480  42.3    469     781

Percentage of the requests served within a certain time (ms)
  50%    469
  66%    469
  75%    484
  80%    484
  90%    484
  95%    531
  98%    719
  99%    781
 100%    781 (longest request)
 

Please don't mark patches RTBC until they're reviewed and tested.

#26 does make a lot of sense to me, but I would like to see that tested. I would not trust PHP to watch my food ;)

@Damz: Here is the output from my system testing the code from #26 in the script provided on #22.

nothing: 0.342183113098 seconds
function no_op(): 2.46541881561 seconds
check plain old: 14.3335449696 seconds
check plain new: 9.67884707451 seconds
check plain new 5.2.5: 6.89955997467 seconds

I meant "test that htmlentities() does the correct thing regarding broken UTF-8 sequences".

htmlspecialchars assumes a default charset of ISO-8859-1 where no multibyte codes exist. In ISO-8859-1 mode, htmlspecialchars does not and cannot care about "invalid" multi-byte sequences.

Drupal strings are UTF-8; You must pass 'UTF-8' as the charset argument to get proper behaviour.

htmlspecialchars("Foo\xC0barbaz", ENT_QUOTES, 'UTF-8'); should return an empty string.

which patch is under consideration? #18? Maybe we should just get that in and have any further discussion to post-freeze.

The code in #26 (no patch) seems to me to be as far as we would want to go in terms of taking advantage of PHP 5.2.5+. We should not require it.

Yes, that was also my gut feeling when I replaced all instances of check_plain() with htmlspecialchars() directly. That won't work out for the "average developer and themer"... Additionally, it seems that the performance gain of that approach is minimal. So let's get back to the BC approach in #26.

Benchmarks, anyone?

What about dynamically defining check_plain(), depending on the result of the PHP version check? This results in the fastest possible implementation for suitable PHP versions, and a slower one for the rest.

I'm a big fan of #42. Defining the function so that we don't have to check the static on each call should save us even more time overall.

#42 omits the basic optimization of duplicating the preg_match

Inlined drupal_validate_utf8() as per #44.

In the doxygen, "Uses drupal_validate_utf8()" is not really true. Maybe "uses the same method..."?

Also, the conditional define is abnormal for Drupal core, so it might be worth explaining in the comments why is doesn't look like http://api.drupal.org/api/function/drupal_clone/6 In oter words, this function is called so often for a non-cached page that any sub-calls add significant overhead and we thus put the preg code there directly adn conditionally define the function to avoid doing version_compare each time it's called.

From #18
+ * check_plain() also validates strings as UTF-8 to prevent cross site scripting
+ * attacks on Internet Explorer 6. We duplicate the preg_match() from
+ * drupal_validate_utf8() here rather than calling the function to avoid
+ * the overhead of an additional function call, since check_plain() may be
+ * called hundreds of times during a request.

This needs to be benchmarked. As far as I know, conditionally defined functions cannot be cached by most (if not all) op-code caches. I personally would recommend the static variable approach.

hmm, well we certainly want the function to be cacheable in the opcode.

If we use a static, it should be a normal static (not use drupal_static()) since it doesn't make sense to me that the PHP version would ever change while the code is executing.

ok, more like #18

Heine points out that I need === to distinguish FALSE from NULL

#51 appears to be about 6% faster than #26, and 33% faster than #18

nothing: 0.330275058746 seconds
function no_op(): 2.31866407394 seconds
check plain old: 14.1262290478 seconds
check plain new: 9.65815281868 seconds
check plain new 5.2.5: 6.80544900894 seconds
check plain new 5.2.5 pwolanin: 6.36292600632 seconds

It would be worthwhile to add #38 as a SimpleTest to ensure that the intended functionality remains intact.

+++ includes/bootstrap.inc
@@ -1120,11 +1120,35 @@ function drupal_unpack($obj, $field = 'data') {
+  static $php525 = NULL;
+  
+  if ($php525 === NULL) {

Just use !isset() like everywhere else in core (and remove the = NULL from the static).

(Also note trailing white-space here)

+++ includes/bootstrap.inc
@@ -1120,11 +1120,35 @@ function drupal_unpack($obj, $field = 'data') {
+    $php525 = version_compare(PHP_VERSION, '5.2.5', '>=');
...
+  if ($php525) {

An additional cast to Boolean may speed up the further tests of the variable.

Otherwise, looks good.

This review is powered by Dreditor.

@sun - I assumed === might be faster than isset(), but I can do it that way.

The return value of version_compare() will be a boolean.

minor fixes per sun, plus a simple unit test case.

nothing: 0.338335990906 seconds
function no_op(): 2.31378507614 seconds
check plain old: 14.4185819626 seconds
check plain new: 9.58456897736 seconds
check plain new 5.2.5: 6.64569997787 seconds
check plain new 5.2.5 pwolanin: 6.31802201271 seconds
check plain new 5.2.5 pwolanin 2nd edition: 6.39244294167 seconds

About 1% slower than #51. Thankfully, isset isn't a function. :)

Thanks pwolanin! Let's get this in soon.

+

I support this approach, but most of the new PHPdoc at the top of the function needs to be moved (back) into code. The reader of the API documentation doesn't (and shouldn't care) about the fact that we inlined some code. Let's do a quick re-roll.

Can we have more tests? With overlong three byte encoding for example?

moved the bulk of the comments to the function body.

some possible test cases can be derived from http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt

@Heine, @pwolanin: Is this what you were thinking?

     $text = check_plain("\xE0\x80\xAF");
     $this->assertEqual($text, '', 'check_plain() rejects invalid sequence "\xE0\x80\xAF"');

Note:
- \x2F succeeds as expected.
- \xC0\x2F fails as expected.
- \xC0\xAF and \xE0\x80\xAF fail in the preg_match as expected, but unexpectedly does not fail in the htmlspecialchars.

The behavior for these 4 test cases is identical in 5.1.6 and 5.2.10, except that in 5.1.6 a PHP Warning is generated for \xC0\x2F from htmlspecialchars.

Perhaps I am doing something wrong; I would appreciate it if someone who knows more about UTF-8 than I do could let me know if this is correct.

We shoudl build a little bigger test set - but basically, if the PHP internals don't handle this as robustly as the preg_match, we should just go back to the simpler earlier patch that doesn't try to rely on PHP version.

I am no expert on this, but here is my understand of this:

AFAICT htmlspecialchars() only rejects partial multibyte sequences followed by a byte < 0x80 (or the end of the string). I think this is fine - the UTF-8 exploit is only relevant if one of the few characters that would otherwise be escaped by htmlspecialchars() (i.e. " < > & ') is "hidden" by a partial multibyte sequence. These few characters all have character codes < 0x80. In other words, htmlspecialchars() does not block all invalid UTF-8 strings, but it does block the dangerous ones.

Here is the change that was made to htmlspecialchars() in PHP 5.2.5:
http://svn.php.net/viewvc/php/php-src/branches/PHP_5_2/ext/standard/html...

Thanks for the link to the diff. So, it seems like we can proceed as long as we are selective about picking test cases? Or would we rather pay the performance penalty to have more consistent drupal behavior across PHP version? I could imagine see this leading to some hard-to-debug problems.

Given the grave performance implications here and the need to backport ASAP I would strongly recommend committing the latest patch and building the test case later. This is an exception to a rule I know but it has good reason.

@c960657: Thank you for the explanation.

+1 to #63 because it doesn't alter the intent of the preg_match, and has significant performance benefits.

so?

@scroogie: I'm pretty sure this is the point where more people review and approve the code or it gets committed. It takes a lot of patience sometimes. :)

Given the last comments, it's entirely not clear to me which patch should be committed to D7. Please re-attach.

Tests failed.

Huh. Somehow I had missed Dries's #60 where he points out that he supports this approach apart from some commenting issues. It looks like those comments have been addressed, along with nicely expanded test coverage. Additionally, it looks like this speeds things up quite a bit, according to the microbenchmarks.

Therefore, committed to HEAD. Seems like it might be worth back-porting this to D6?

And here's a backport for d6

Didn't read through whole issue, but does this fix or is aware of:

htmlspecialchars(): Invalid multibyte sequence in argument	Warning	bootstrap.inc	1204	check_plain()

when run on PHP 5.3.

@boombatower not this patch not about this warning, suppose test should be changed a bit

Also why this improvement still not in?

I pointed it out to Gabor today - we just need final confirmation that it's working cleanly on D6.

Patch applies cleanly and matches what's already in D7.

For maintenance reasons, I'd add a strong, very visible reference to check_plain() in drupal_validate_utf8(), so if we need modification there, we don't forget to roll it to here as well. When we have multiple copies of code around, we should make sure to modify them in concert.

@Gábor Hojtsy purpose of drupal_validate_utf8() is different - just to check a string. I see nothing to modify in drupal_validate_utf8()

@andypost: imagine drupal_validate_utf8() will have a change in a month or a year or two. Will people making that change remember that the same code appears in check_plain() and needs modification? I'm saying we should add a clear comment to drupal_validate_utf8() to notify people in the future that the same code is copied in check_plain().

Added an inline comment in validate_utf8() as requested in #88.

Suppose this comment is enough

There's a follow up for D7 #755758: check_plain throwing warnings

Committed, thanks.

apostrophe is being encoded to html code when used with Advanced Forum module's navigation. If a topic has an apostrophe, say for example, CiCi's Pizza, it's displaying the link as: CiCi's Pizza

Does anyone have any suggestions as to a fix for this?

After this patch #91 was commited to D6.17 a lot such kind of questions "warning: htmlspecialchars() [function.htmlspecialchars]: Invalid multibyte sequence" you can find on forum.

I had to replace in my latest D6.19 file bootstrap.inc with same file from D6.16 to get rid of this warning error.