Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPACK patch broken for Nginx 1.13.10 #93

Open
centminmod opened this issue Mar 20, 2018 · 31 comments
Open

HPACK patch broken for Nginx 1.13.10 #93

centminmod opened this issue Mar 20, 2018 · 31 comments

Comments

@centminmod
Copy link

Using @PikachuEXE patch #92

patching file auto/modules
Hunk #1 succeeded at 438 (offset 1 line).
patching file auto/options
Hunk #2 succeeded at 224 (offset 1 line).
Hunk #3 succeeded at 436 (offset 2 lines).
patching file src/core/ngx_murmurhash.c
patching file src/core/ngx_murmurhash.h
patching file src/http/v2/ngx_http_v2.c
Hunk #1 succeeded at 270 (offset -2 lines).
Hunk #2 succeeded at 2056 (offset 1 line).
patching file src/http/v2/ngx_http_v2.h
Hunk #1 succeeded at 52 (offset 1 line).
Hunk #2 succeeded at 123 (offset 1 line).
Hunk #3 succeeded at 178 (offset 1 line).
Hunk #4 succeeded at 207 (offset 1 line).
Hunk #5 succeeded at 465 with fuzz 2 (offset 53 lines).
patching file src/http/v2/ngx_http_v2_filter_module.c
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 93.
Hunk #3 succeeded at 155 (offset -41 lines).
Hunk #4 succeeded at 433 (offset -41 lines).
Hunk #5 succeeded at 441 (offset -41 lines).
Hunk #6 succeeded at 449 (offset -41 lines).
Hunk #7 succeeded at 467 (offset -41 lines).
Hunk #8 succeeded at 514 (offset -41 lines).
Hunk #9 succeeded at 566 with fuzz 1 (offset -41 lines).
Hunk #10 FAILED at 1039.
Hunk #11 FAILED at 1065.
4 out of 11 hunks FAILED -- saving rejects to file src/http/v2/ngx_http_v2_filter_module.c.rej
patching file src/http/v2/ngx_http_v2_table.c
Hunk #1 succeeded at 361 (offset 14 lines).
@kn007
Copy link

kn007 commented Mar 20, 2018

Maybe you can try my patch, https://github.com/kn007/patch/blob/master/nginx.patch

@centminmod
Copy link
Author

have you tried with nginx 1.3.10 master branch you patch as huge amount of changes in src/http/v2/ngx_http_v2_filter_module.c at https://github.com/nginx/nginx/blob/master/src/http/v2/ngx_http_v2_filter_module.c

@kn007
Copy link

kn007 commented Mar 20, 2018

Oh, sorry. My mistake.
I do not test it on 1.13.10.

@kn007
Copy link

kn007 commented Mar 22, 2018

@centminmod
Copy link
Author

Thanks @kn007 seems your patch is 3 in one for

Add SPDY Support.
Add HTTP2 HPACK Encoding Support.
Add Dynamic TLS Record support.

Do you have HTTP HPACK patch by itself ?

@kn007
Copy link

kn007 commented Mar 23, 2018

@centminmod I'm off work now. I'll make patch for you when I get home.

kn007 added a commit to kn007/patch that referenced this issue Mar 23, 2018
@kn007
Copy link

kn007 commented Mar 23, 2018

https://github.com/kn007/patch/blob/master/nginx__1.13.10_http2_hpack.patch

@centminmod
Copy link
Author

Thanks @kn007 it patched perfectly in 1.13.10. Very much appreciate your help with this !

patching file auto/modules
patching file auto/options
patching file src/core/ngx_murmurhash.c
patching file src/core/ngx_murmurhash.h
patching file src/http/v2/ngx_http_v2.c
patching file src/http/v2/ngx_http_v2_encode.c
patching file src/http/v2/ngx_http_v2_filter_module.c
patching file src/http/v2/ngx_http_v2.h
patching file src/http/v2/ngx_http_v2_table.c

compiled fine


nginx -V
nginx version: nginx/1.13.10
built by gcc 8.0.1 20180318 (experimental) (GCC) 
built with OpenSSL 1.1.1-pre3 (beta) 20 Mar 2018
TLS SNI support enabled

configure arguments: --with-ld-opt='-L/usr/local/lib -ljemalloc -Wl,-z,relro -Wl,-rpath,/usr/local/lib' --with-cc-opt='-I/usr/local/include -m64 -march=native -DTCP_FASTOPEN=23 -g -O3 -Wno-error=strict-aliasing -fstack-protector-strong -flto -fuse-ld=gold --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wimplicit-fallthrough=0 -fcode-hoisting -Wno-cast-function-type -Wp,-D_FORTIFY_SOURCE=2 -Wno-deprecated-declarations' --sbin-path=/usr/local/sbin/nginx --conf-path=/usr/local/nginx/conf/nginx.conf --with-compat --with-http_stub_status_module --with-http_secure_link_module --add-dynamic-module=../nginx-module-vts --with-libatomic --with-http_gzip_static_module --add-dynamic-module=../ngx_brotli --with-http_sub_module --with-http_addition_module --with-http_image_filter_module=dynamic --with-http_geoip_module --with-stream_geoip_module --with-stream_realip_module --with-stream_ssl_preread_module --with-threads --with-stream=dynamic --with-stream_ssl_module --with-http_realip_module --add-dynamic-module=../ngx-fancyindex-0.4.2 --add-module=../ngx_cache_purge-2.4.2 --add-module=../ngx_devel_kit-0.3.0 --add-dynamic-module=../set-misc-nginx-module-0.31 --add-dynamic-module=../echo-nginx-module-0.61 --add-module=../redis2-nginx-module-0.14 --add-module=../ngx_http_redis-0.3.7 --add-module=../memc-nginx-module-0.18 --add-module=../srcache-nginx-module-0.31 --add-dynamic-module=../headers-more-nginx-module-0.33 --with-pcre=../pcre-8.42 --with-pcre-jit --with-zlib=../zlib-cloudflare-1.3.0 --with-http_ssl_module --with-http_v2_module --with-http_v2_hpack_enc --with-openssl=../openssl-1.1.1-pre3 --with-openssl-opt='enable-ec_nistp_64_gcc_128 enable-tls1_3'

h2load test HTTP/2 HPACK Full Encoding = 93.7+ % header space savings and counting 👍

url=https://http2.domain.com

for i in $(seq 1 20); do echo "h2load run $i"; h2load $url -n $i | tail -6 | head -1; done   
h2load run 1
traffic: 6.29KB (6444) total, 344B (344) headers (space savings 36.88%), 5.89KB (6033) data
h2load run 2
traffic: 12.22KB (12513) total, 362B (362) headers (space savings 66.79%), 11.78KB (12066) data
h2load run 3
traffic: 18.15KB (18582) total, 380B (380) headers (space savings 76.76%), 17.67KB (18099) data
h2load run 4
traffic: 24.07KB (24651) total, 398B (398) headers (space savings 81.74%), 23.57KB (24132) data
h2load run 5
traffic: 30.00KB (30720) total, 416B (416) headers (space savings 84.73%), 29.46KB (30165) data
h2load run 6
traffic: 35.93KB (36789) total, 434B (434) headers (space savings 86.73%), 35.35KB (36198) data
h2load run 7
traffic: 41.85KB (42858) total, 452B (452) headers (space savings 88.15%), 41.24KB (42231) data
h2load run 8
traffic: 47.78KB (48927) total, 470B (470) headers (space savings 89.22%), 47.13KB (48264) data
h2load run 9
traffic: 53.71KB (54996) total, 488B (488) headers (space savings 90.05%), 53.02KB (54297) data
h2load run 10
traffic: 59.63KB (61065) total, 506B (506) headers (space savings 90.72%), 58.92KB (60330) data
h2load run 11
traffic: 65.56KB (67134) total, 524B (524) headers (space savings 91.26%), 64.81KB (66363) data
h2load run 12
traffic: 71.49KB (73203) total, 542B (542) headers (space savings 91.71%), 70.70KB (72396) data
h2load run 13
traffic: 77.41KB (79272) total, 560B (560) headers (space savings 92.10%), 76.59KB (78429) data
h2load run 14
traffic: 83.34KB (85341) total, 578B (578) headers (space savings 92.42%), 82.48KB (84462) data
h2load run 15
traffic: 89.27KB (91410) total, 596B (596) headers (space savings 92.71%), 88.37KB (90495) data
h2load run 16
traffic: 95.19KB (97479) total, 614B (614) headers (space savings 92.96%), 94.27KB (96528) data
h2load run 17
traffic: 101.12KB (103548) total, 632B (632) headers (space savings 93.18%), 100.16KB (102561) data
h2load run 18
traffic: 107.05KB (109617) total, 650B (650) headers (space savings 93.37%), 106.05KB (108594) data
h2load run 19
traffic: 112.97KB (115686) total, 668B (668) headers (space savings 93.55%), 111.94KB (114627) data
h2load run 20
traffic: 118.90KB (121755) total, 686B (686) headers (space savings 93.71%), 117.83KB (120660) data

@kn007
Copy link

kn007 commented Mar 25, 2018

You are welcome.

@PikachuEXE
Copy link

I test with https://http2.domain.com with h2load but space saving is 0%
Is it reverted?

I test the patch with my own website but IE11 in win10 does not like it :P

@centminmod
Copy link
Author

@PikachuEXE what's happening in IE11 ?

@PikachuEXE
Copy link

It won't load after first request

@kevin25
Copy link

kevin25 commented Apr 3, 2018

I tested on IE 11 Windows 7 and worked for me.

@PikachuEXE
Copy link

What website do you use for testing?

@PikachuEXE
Copy link

PikachuEXE commented Apr 3, 2018

I test the patch with our own website with BrowserStack IE11 on Win7 again
The status code is HTTP/1.0 503 Connect failed

edit: this error is caused by incorrect Nginx config, let me post the actual error later

@PikachuEXE
Copy link

OK this is our testing website: https://staging.spacious.hk/en/hong-kong/for-sale
Currently using Nginx with the HPACK patch applied

IE11 on Win7 on BrowserStack sometimes crashes, which is why I am not sure if it's working for IE 11 or not

@kevin25
Copy link

kevin25 commented Apr 3, 2018

screen shot 2018-04-03 at 05 33 58

Is it?

@kn007
Copy link

kn007 commented Apr 3, 2018

Using win7 system with IE 11, it was working fine.

@PikachuEXE
Copy link

Thanks guys :)
Maybe I should try to create a windows VM for testing instead _

Oh 1.13.11 is out, need to test again :3

@PikachuEXE
Copy link

Tested with Virtual box VM with IE11 on win 7
The patch works fine for Nginx 1.13.11

$ url=https://www.spacious.hk/en/about-us
$ for i in $(seq 1 10); do echo "h2load run $i"; h2load $url -n $i | tail -6 | head -1; done
h2load run 1
traffic: 190.49KB (195062) total, 859B (859) headers (space savings 28.42%), 189.37KB (193911) data
h2load run 2
traffic: 380.51KB (389641) total, 1.25KB (1283) headers (space savings 46.47%), 378.73KB (387823) data
h2load run 3
traffic: 570.70KB (584396) total, 1.86KB (1901) headers (space savings 47.21%), 568.10KB (581735) data
h2load run 4
traffic: 760.60KB (778851) total, 2.13KB (2183) headers (space savings 54.48%), 757.47KB (775647) data
h2load run 5
traffic: 951.00KB (973824) total, 2.95KB (3020) headers (space savings 49.64%), 946.83KB (969558) data
h2load run 6
traffic: 1.11MB (1168562) total, 3.53KB (3613) headers (space savings 49.81%), 1.11MB (1163469) data
h2load run 7
traffic: 1.30MB (1363166) total, 3.96KB (4052) headers (space savings 51.76%), 1.29MB (1357382) data
h2load run 8
traffic: 1.49MB (1557747) total, 4.37KB (4478) headers (space savings 53.32%), 1.48MB (1551294) data
h2load run 9
traffic: 1.67MB (1752568) total, 5.04KB (5159) headers (space savings 52.20%), 1.66MB (1745200) data
h2load run 10
traffic: 1.86MB (1947381) total, 5.71KB (5844) headers (space savings 51.29%), 1.85MB (1939112) data

@kevin25
Copy link

kevin25 commented Apr 18, 2018

Any idea if it works with Nginx 1.14.0?

@kevin25
Copy link

kevin25 commented Apr 18, 2018

It failed for me

patching file auto/modules
Hunk #1 succeeded at 438 (offset 1 line).
patching file auto/options
Hunk #2 succeeded at 224 (offset 1 line).
Hunk #3 succeeded at 436 (offset 2 lines).
patching file src/core/ngx_murmurhash.c
patching file src/core/ngx_murmurhash.h
patching file src/http/v2/ngx_http_v2.c
Hunk #1 succeeded at 270 (offset -2 lines).
Hunk #2 succeeded at 2056 (offset 1 line).
patching file src/http/v2/ngx_http_v2.h
Hunk #1 succeeded at 52 (offset 1 line).
Hunk #2 succeeded at 123 (offset 1 line).
Hunk #3 succeeded at 178 (offset 1 line).
Hunk #4 succeeded at 207 (offset 1 line).
Hunk #5 succeeded at 465 with fuzz 2 (offset 53 lines).
patching file src/http/v2/ngx_http_v2_filter_module.c
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 93.
Hunk #3 succeeded at 155 (offset -41 lines).
Hunk #4 succeeded at 433 (offset -41 lines).
Hunk #5 succeeded at 441 (offset -41 lines).
Hunk #6 succeeded at 449 (offset -41 lines).
Hunk #7 succeeded at 467 (offset -41 lines).
Hunk #8 succeeded at 514 (offset -41 lines).
Hunk #9 succeeded at 566 with fuzz 1 (offset -41 lines).
Hunk #10 FAILED at 1039.
Hunk #11 FAILED at 1065.
4 out of 11 hunks FAILED -- saving rejects to file src/http/v2/ngx_http_v2_filter_module.c.rej
patching file src/http/v2/ngx_http_v2_table.c
Hunk #1 succeeded at 361 (offset 14 lines).

@kn007
Copy link

kn007 commented Apr 18, 2018

@kevin25 Even using my patch?
It seems that these hunk errors are not caused by my patch.

You could using this one: https://github.com/kn007/patch/blob/master/nginx__1.13.10_http2_hpack.patch

Pls delete the old files and re-extract the nginx-1.14.0.tar.gz, then patch the patch to try it again.

@kevin25
Copy link

kevin25 commented Apr 19, 2018

Yes, i did. This one seems to work. I'll test it. Thank you. Cloudflare team doesn't support this anymore?

@anotherjin
Copy link

anotherjin commented Apr 19, 2018 via email

@PikachuEXE
Copy link

Cloudflare open-source the patch but don't really maintain for every Nginx version

@kevin25
Copy link

kevin25 commented Jun 1, 2019

@PikachuEXE
Copy link

@kevin25
Copy link

kevin25 commented Oct 27, 2019

@kevin25
Copy link

kevin25 commented Oct 27, 2019

I am using https://github.com/hakasenyang/openssl-patch/blob/master/nginx_hpack_push_1.15.3.patch for nginx 1.17.0

Thank you. Is it still working with 1.17.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants