Implements double_encode in Htmlspecialchars #1862

k-fish · 2014-02-18T22:36:53Z

Implements double_encode check at '&' char when iterating over string. Identifies utf-8 entities by default (PHP 5.4+).

Fixes htmlspecialchars not respecting $double_encode parameter #1857

k-fish · 2014-02-20T02:25:26Z

da2f384 will break tests in ext_string , but those tests are invalid (along with a couple others in ext_string.php). Will make a PR to update ext_string test.

ptarjan · 2014-02-20T05:24:56Z

@k-fish you can bundle that change in here since I will need to flatten them to merge this (as all tests have to pass to land a PR)

This reverts commit d6f6538. Problem was not universal

ptarjan · 2014-02-21T01:10:38Z

Here are the differences I see: https://gist.github.com/ptarjan/9126870

This reverts commit da2f384.

k-fish · 2014-02-23T00:45:43Z

This should fix one of the 2 bugs remaining

ptarjan · 2014-02-23T03:13:07Z

I'm seeing 3 failures still. (and they are also on travis)

test/slow/ext_string/htmlspecialchars_utf8.php
--- test/slow/ext_string/htmlspecialchars_utf8.php.expect-trimmed       2014-02-22 19:11:46.402759358 -0800
+++ test/slow/ext_string/htmlspecialchars_utf8.php.out  2014-02-22 19:11:46.401759346 -0800
@@ -1,5 +1,5 @@
 'Foo�barbaz' => ''
 '�"' => ''
-' ' => ' '
+' ' => '&nbsp;'
 '�' => ''
 'Fooÿñ' => 'Fooÿñ'
\ No newline at end of file

test/slow/ext_string/ext_string.php
--- test/slow/ext_string/ext_string.php.expect-trimmed  2014-02-22 19:11:46.402759358 -0800
+++ test/slow/ext_string/ext_string.php.out     2014-02-22 19:11:46.402759358 -0800
@@ -89,7 +89,7 @@
 string(10) "-=-=-Alien"
 string(10) "__Alien___"
 string(6) "Alien_"
-string(6) "Alien�"
+string(6) "Alien"
 string(20) "-=-=-=-=-=-=-=-=-=-="
 string(57) "The quick brown fox<br />
 jumped over the lazy<br />
@@ -133,8 +133,8 @@
 string(9) "AtestB10C"
 string(10) "0000001101"
 string(2) "09"
-string(17) "(foo�bar-bar�foo)"
-string(5) "[a�b]"
+string(17) "(foobar-barfoo)"
+string(5) "[ab]"
 string(9) "AtestB10C"
 array(1) {
   [0]=>
@@ -199,8 +199,7 @@
 int(0)
 int(7)
 bool(false)
-
-Warning: strpos(): Empty needle in /tmp/a.php on line 377
+HipHop Warning: Empty delimiter in /data/fusion/ptarjan/fbcode5/hphp/test/slow/ext_string/ext_string.php on line 377
 bool(false)
 int(7)
 int(7)
.
test/slow/ext_string/htmlspecialchars.php
--- test/slow/ext_string/htmlspecialchars.php.expect-trimmed    2014-02-22 19:11:46.411759463 -0800
+++ test/slow/ext_string/htmlspecialchars.php.out       2014-02-22 19:11:46.410759451 -0800
@@ -4,8 +4,72 @@
 bool(true)
 bool(true)
 bool(true)
-bool(true)
-bool(true)
+bool(false)
+Failed: c2a0
+Got: 266e6273703b
+array(2) {
+  [0]=>
+  array(4) {
+    ["file"]=>
+    string(75) "/data/fusion/ptarjan/fbcode5/hphp/test/slow/ext_string/htmlspecialchars.php"
+    ["line"]=>
+    int(31)
+    ["function"]=>
+    string(2) "VS"
+    ["args"]=>
+    array(2) {
+      [0]=>
+      string(12) "266e6273703b"
+      [1]=>
+      string(4) "c2a0"
+    }
+  }
+  [1]=>
+  array(4) {
+    ["file"]=>
+    string(75) "/data/fusion/ptarjan/fbcode5/hphp/test/slow/ext_string/htmlspecialchars.php"
+    ["line"]=>
+    int(84)
+    ["function"]=>
+    string(21) "test_htmlspecialchars"
+    ["args"]=>
+    array(0) {
+    }
+  }
+}
+bool(false)
+Failed: c2a0
+Got: 266e6273703b
+array(2) {
+  [0]=>
+  array(4) {
+    ["file"]=>
+    string(75) "/data/fusion/ptarjan/fbcode5/hphp/test/slow/ext_string/htmlspecialchars.php"
+    ["line"]=>
+    int(32)
+    ["function"]=>
+    string(2) "VS"
+    ["args"]=>
+    array(2) {
+      [0]=>
+      string(12) "266e6273703b"
+      [1]=>
+      string(4) "c2a0"
+    }
+  }
+  [1]=>
+  array(4) {
+    ["file"]=>
+    string(75) "/data/fusion/ptarjan/fbcode5/hphp/test/slow/ext_string/htmlspecialchars.php"
+    ["line"]=>
+    int(84)
+    ["function"]=>
+    string(21) "test_htmlspecialchars"
+    ["args"]=>
+    array(0) {
+    }
+  }
+}
 bool(true)
 bool(true)
 bool(true)

3 tests failed

See the diffs:
cat test/slow/ext_string/ext_string.php.diff
cat test/slow/ext_string/htmlspecialchars.php.diff
cat test/slow/ext_string/htmlspecialchars_utf8.php.diff

For xargs, list of failures is available using:
cat /tmp/test-failuresIElhEy

Run these by hand:
../_bin/hphp/hhvm/hhvm --config test/slow/config.hdf  -vRepo.Local.Mode=-- -vRepo.Central.Path=/data/fusion/ptarjan/fbcode5/_bin/verify.hhbc -vEval.Jit=true -vEval.EnableArgsInBacktraces=true    -vResourceLimit.CoreFileSize=0 --file 'test/slow/ext_string/ext_string.php' 
../_bin/hphp/hhvm/hhvm --config test/slow/config.hdf  -vRepo.Local.Mode=-- -vRepo.Central.Path=/data/fusion/ptarjan/fbcode5/_bin/verify.hhbc -vEval.Jit=true -vEval.EnableArgsInBacktraces=true    -vResourceLimit.CoreFileSize=0 --file 'test/slow/ext_string/htmlspecialchars.php' 
../_bin/hphp/hhvm/hhvm --config test/slow/config.hdf  -vRepo.Local.Mode=-- -vRepo.Central.Path=/data/fusion/ptarjan/fbcode5/_bin/verify.hhbc -vEval.Jit=true -vEval.EnableArgsInBacktraces=true    -vResourceLimit.CoreFileSize=0 --file 'test/slow/ext_string/htmlspecialchars_utf8.php' 

Re-run just the failing tests:
test/run test/slow/ext_string/ext_string.php test/slow/ext_string/htmlspecialchars.php test/slow/ext_string/htmlspecialchars_utf8.php

k-fish · 2014-02-23T10:23:25Z

I've got this to one bug left on my end, have to flip entity maps to encode to html entities.

Updated test to expectf, @ptarjan, what am I doing wrong that the %s is not working for the warning?

ptarjan · 2014-02-23T17:51:43Z

@k-fish first you should rebase on master, since i think that will help a lot of the spurious travis failures. What string-mismatch are you talking about? If the test is failing for some reason, the thing it prints is just a straight diff. Ignore any of the mismatches with % in them since diff doesn't know how to match them.

k-fish · 2014-02-23T22:15:18Z

@ptarjan got the last one, now all tests in ext_string pass on my end. I still get Repostmt errors in TravisCI though.

ptarjan · 2014-02-24T07:54:45Z

I'm also seeing asan aborts with stack buffer overflow in test/zend/good/ext/standard/tests/strings/crypt.php.

 #2 0x3f4df95 in HPHP::StringUtil::Crypt(HPHP::String const&, char const*) hphp/runtime/base/string-util.cpp:438

I'll re-run with your merge I guess

ptarjan · 2014-02-25T01:50:39Z

Almost there. You still have ASAN failures:

+=================================================================
  +==46576== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60040032379d at pc 0x5b040d2 bp 0x7fff2fb43150 sp 0x7fff2fb43148
  +WRITE of size 1 at 0x60040032379d thread T0
  +    #0 0x5b040d1 in HPHP::string_html_encode(char const*, int&, long, bool, bool, bool) hphp/zend/zend-html.cpp:776
  +    #1 0x3f5944a in HPHP::StringUtil::HtmlEncode(HPHP::String const&, long, char const*, bool, bool) hphp/runtime/base/string-util.cpp:220
  +    #2 0x389d473 in HPHP::f_htmlentities(HPHP::String const&, int, HPHP::String const&, bool) hphp/runtime/ext/ext_string.cpp:1318
  +    #3 0x286d5f9 in HPHP::fg_htmlentities(HPHP::ActRec*) _build/opt/hphp/runtime/ext_hhvm/32d97e8/ext/ext_string.ext_hhvm.cpp:2719
  +    #4 0x4be778a (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x4be778a)
  +    #5 0x101420b (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x101420b)
  +    #6 0x1034955 (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x1034955)
  +    #7 0x4c1d7a6 (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x4c1d7a6)
  +    #8 0x3ea9382 in invoke_file_impl hphp/runtime/base/builtin-functions.cpp:910
  +    #9 0x141fdf9 in HPHP::hphp_invoke(HPHP::ExecutionContext*, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool, HPHP::Array const&, HPHP::VRefParamValue const&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> >&, bool, bool, bool) hphp/runtime/base/program-functions.cpp:1652
  +    #10 0x3f34d49 in HPHP::hphp_invoke_simple(std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool) hphp/runtime/base/program-functions.cpp:1621
  +    #11 0x3f4ef3a in HPHP::execute_program_impl(int, char**) hphp/runtime/base/program-functions.cpp:1369
  +    #12 0x3f51f4b in HPHP::execute_program(int, char**) hphp/runtime/base/program-functions.cpp:856
  +    #13 0x14bd3e6 in main hphp/hhvm/main.cpp:57
  +    #14 0x7f83cfe28efe in __libc_start_main (/usr/local/fbcode/gcc-4.8.1-glibc-2.17-fb/lib/libc.so.6+0x21efe)
  +    #15 0x5f269b4 in _start /home/engshare/third-party2/glibc/2.17/src/glibc-2.17/sysdeps/x86_64/start.S:123
  +0x60040032379d is located 0 bytes to the right of 13-byte region [0x600400323790,0x60040032379d)
  +allocated by thread T0 here:
  +    #0 0x7f83d50b9f4a in __interceptor_malloc (/usr/local/fbcode/gcc-4.8.1-glibc-2.17-fb/lib/libasan.so.0+0x17f4a)
  +    #1 0x5b03144 in HPHP::string_html_encode(char const*, int&, long, bool, bool, bool) hphp/zend/zend-html.cpp:599
  +    #2 0x3f5944a in HPHP::StringUtil::HtmlEncode(HPHP::String const&, long, char const*, bool, bool) hphp/runtime/base/string-util.cpp:220
  +    #3 0x389d473 in HPHP::f_htmlentities(HPHP::String const&, int, HPHP::String const&, bool) hphp/runtime/ext/ext_string.cpp:1318
  +    #4 0x286d5f9 in HPHP::fg_htmlentities(HPHP::ActRec*) _build/opt/hphp/runtime/ext_hhvm/32d97e8/ext/ext_string.ext_hhvm.cpp:2719
  +    #5 0x4be778a (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x4be778a)
  +    #6 0x101420b (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x101420b)
  +    #7 0x1034955 (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x1034955)
  +    #8 0x4c1d7a6 (/data/contbuild/slave/_sharedbuilddir/build/_build/opt/hphp/hhvm/hhvm+0x4c1d7a6)
  +    #9 0x3ea9382 in invoke_file_impl hphp/runtime/base/builtin-functions.cpp:910
  +    #10 0x141fdf9 in HPHP::hphp_invoke(HPHP::ExecutionContext*, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool, HPHP::Array const&, HPHP::VRefParamValue const&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool&, std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> >&, bool, bool, bool) hphp/runtime/base/program-functions.cpp:1652
  +    #11 0x3f34d49 in HPHP::hphp_invoke_simple(std::basic_fbstring<char, std::char_traits<char>, std::allocator<char>, std::fbstring_core<char> > const&, bool) hphp/runtime/base/program-functions.cpp:1621
  +    #12 0x3f4ef3a in HPHP::execute_program_impl(int, char**) hphp/runtime/base/program-functions.cpp:1369
  +    #13 0x3f51f4b in HPHP::execute_program(int, char**) hphp/runtime/base/program-functions.cpp:856
  +    #14 0x14bd3e6 in main hphp/hhvm/main.cpp:57
  +    #15 0x7f83cfe28efe in __libc_start_main (/usr/local/fbcode/gcc-4.8.1-glibc-2.17-fb/lib/libc.so.6+0x21efe)
  +    #16 0x5f269b4 in _start /home/engshare/third-party2/glibc/2.17/src/glibc-2.17/sysdeps/x86_64/start.S:123
  +SUMMARY: AddressSanitizer: heap-buffer-overflow hphp/zend/zend-html.cpp:673 HPHP::string_html_encode(char const*, int&, long, bool, bool, bool)
  +Shadow bytes around the buggy address:
  +  0x0c010005c6a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  +  0x0c010005c6b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  +  0x0c010005c6c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  +  0x0c010005c6d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  +  0x0c010005c6e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  +=>0x0c010005c6f0: fa fa 00[05]fa fa fd fa fa fa fd fa fa fa fd fa
  +  0x0c010005c700: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fa
  +  0x0c010005c710: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  +  0x0c010005c720: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  +  0x0c010005c730: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  +  0x0c010005c740: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  +Shadow byte legend (one shadow byte represents 8 application bytes):
  +  Addressable:           00
  +  Partially addressable: 01 02 03 04 05 06 07 
  +  Heap left redzone:     fa
  +  Heap righ redzone:     fb
  +  Freed Heap region:     fd
  +  Stack left redzone:    f1
  +  Stack mid redzone:     f2
  +  Stack right redzone:   f3
  +  Stack partial redzone: f4
  +  Stack after return:    f5
  +  Stack use after scope: f8
  +  Global redzone:        f9
  +  Global init order:     f6
  +  Poisoned by user:      f7
  +  ASan internal:         fe
  +==46576== ABORTING

Summary: X-link: facebook/folly#1862 Avoid calling clock::now in the common case. Clock::now() can be an expensive call (a call to libc's __clock_gettime). PGO data shows that the first check almost always succeeds, which makes the call to Clock::now redundant. This patch peels one iteration that calls f() before calling clock::now(). Notice that a very common use of this API is a function that performs a single load in the function, so timing is not a major concern. Reviewed By: magedm Differential Revision: D39611860 fbshipit-source-id: 8c7affc83f1e86fe3c8b2401391e1159d08a99ec

K Fisher added 4 commits February 18, 2014 14:40

Enabled double encode feature for htmlspecialchars

c6dc668

Double_encode now gets passed into function

4383173

Updated tests .expect

1cf6b9a

Encode for utf-8 set

ef306ad

scannell assigned ptarjan Feb 18, 2014

Removed unicode case that doesn't exist in Zend PHP

da2f384

K Fisher added 8 commits February 20, 2014 00:39

Fixed ext_string tests so that they rely on php's actual expected

fc2eaff

Fixed var_dump output with arrays not including space

d6f6538

Fixed a few ext_string functions to use UTF by default

faace17

Partial update of ext_string

3560119

Merge remote-tracking branch 'upstream/master' into htmlspecialchars_fix

bb7b9a2

Revert "Fixed var_dump output with arrays not including space"

6c2a937

This reverts commit d6f6538. Problem was not universal

Fixed json to make UTF-8 default encoding

9c0eec6

PHP>=5.3 crypt unspecified is md5

44cd76d

K Fisher added 2 commits February 22, 2014 19:42

Fixed strcmp to match Zend

604e5dd

Revert "Removed unicode case that doesn't exist in Zend PHP"

edaef64

This reverts commit da2f384.

Cleaned up prior changes, fixed strcmp(), and test.

347e6f0

Merge remote-tracking branch 'upstream/master' into htmlspecialchars_fix

2a839cf

Added support for encoding htmlentities

2d31849

K Fisher added 2 commits February 24, 2014 04:15

Added null to buffer in crypt()

21a257c

Fixed index

30bc80d

Expanded return buffer to allow for larger entity names

77654c5

sgolemon closed this in 77d1ea3 Mar 2, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements double_encode in Htmlspecialchars #1862

Implements double_encode in Htmlspecialchars #1862

k-fish commented Feb 18, 2014

k-fish commented Feb 20, 2014

ptarjan commented Feb 20, 2014

ptarjan commented Feb 21, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 23, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 23, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 24, 2014

ptarjan commented Feb 25, 2014

Implements double_encode in Htmlspecialchars #1862

Implements double_encode in Htmlspecialchars #1862

Conversation

k-fish commented Feb 18, 2014

k-fish commented Feb 20, 2014

ptarjan commented Feb 20, 2014

ptarjan commented Feb 21, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 23, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 23, 2014

k-fish commented Feb 23, 2014

ptarjan commented Feb 24, 2014

ptarjan commented Feb 25, 2014