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.6 #83

Open
centminmod opened this issue Oct 10, 2017 · 26 comments
Open

HPACK patch broken for Nginx 1.13.6 #83

centminmod opened this issue Oct 10, 2017 · 26 comments

Comments

@centminmod
Copy link

Seems Nginx 1.13.6 update broken HPACK patch https://community.centminmod.com/threads/nginx-announce-nginx-1-13-6.13079/#post-55493

                -o objs/src/http/ngx_http_postpone_filter_module.o \
                src/http/ngx_http_postpone_filter_module.c
src/http/v2/ngx_http_v2_filter_module.c: In function ‘ngx_http_v2_header_filter’:
src/http/v2/ngx_http_v2_filter_module.c:137:32: error: redeclaration of ‘h2c’ with no linkage
     ngx_http_v2_connection_t  *h2c;
                                ^~~
src/http/v2/ngx_http_v2_filter_module.c:133:32: note: previous declaration of ‘h2c’ was here
     ngx_http_v2_connection_t  *h2c;
                                ^~~
make[1]: *** [objs/src/http/v2/ngx_http_v2_filter_module.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/svr-setup/nginx-1.13.6'
make: *** [build] Error 2

after disabling HPACK patch, Nginx 1.13.6 compiled fine on CentOS 6.9 64bit

nginx -V
nginx version: nginx/1.13.6
built by gcc 6.3.1 20170216 (Red Hat 6.3.1-3) (GCC) 
built with OpenSSL 1.1.0f  25 May 2017
TLS SNI support enabled

configure arguments: --with-ld-opt='-ljemalloc -Wl,-z,relro -Wl,-rpath,/usr/local/lib' --with-cc-opt='-m64 -march=native -DTCP_FASTOPEN=23 -g -O3 -fstack-protector-strong -flto -fuse-ld=gold --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -gsplit-dwarf' --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 --with-libatomic --with-http_gzip_static_module --add-dynamic-module=../ngx_pagespeed-1.12.34.3-stable --with-http_sub_module --with-http_addition_module --with-http_image_filter_module=dynamic --with-http_geoip_module=dynamic --with-stream_geoip_module=dynamic --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.32 --with-pcre=../pcre-8.41 --with-pcre-jit --with-zlib=../zlib-1.2.11 --with-http_ssl_module --with-http_v2_module --with-openssl=../openssl-1.1.0f --with-openssl-opt='enable-ec_nistp_64_gcc_128'

@kn007
Copy link

kn007 commented Oct 11, 2017

Same ERROR here

nginx version: nginx/1.13.5
built by gcc 6.3.1 20170216 (Red Hat 6.3.1-3) (GCC)
built with OpenSSL 1.0.2l  25 May 2017
TLS SNI support enabled

Nginx 1.13.6 has already declaration of ‘h2c’.

@pamamolf
Copy link

pamamolf commented Oct 11, 2017

Same error for me also:

nginx version: nginx/1.13.6
built by gcc 6.3.1 20170216 (Red Hat 6.3.1-3) (GCC)
built with LibreSSL 2.5.5
TLS SNI support enabled

@injust
Copy link
Contributor

injust commented Oct 13, 2017

Removing line 324 from the patch will make it work.

For a version of the HPACK patch that works on nginx 1.13.6, see https://github.com/Injust/hws/blob/master/individual-patches/nginx__http2-hpack.patch.

@HansVanEijsden
Copy link

Thanks for fixing! 😃 But unfortunately the patch is giving me problems now, with Nginx 1.13.6. At random I get curl errors: "Stream error in the HTTP/2 framing layer" with images. And, Chrome works fine but Safari (newest, on iOS and macOS) sometimes also doesn't load images. After doing a clean build without the patch, all is working again.

The clean working build without your patch:
nginx version: nginx/1.13.6 built by gcc 6.3.0 20170516 (Debian 6.3.0-18) built with OpenSSL 1.1.0f 25 May 2017 TLS SNI support enabled configure arguments: --add-module=/usr/local/src/ngx_brotli_module --add-module=/home/hans/ngx_pagespeed-1.12.34.3-stable --prefix=/opt/nginx --user=www-data --group=www-data --with-http_ssl_module --with-http_v2_module --with-openssl=/usr/local/src/openssl-1.1.0f --with-openssl-opt='enable-ec_nistp_64_gcc_128 -DCFLAGS='-O3 -march=native -flto -fuse-linker-plugin'' --with-pcre-jit --with-file-aio --with-http_flv_module --with-http_geoip_module --with-http_gunzip_module --with-http_mp4_module --with-http_realip_module --with-http_stub_status_module --with-threads --with-libatomic --add-module=/usr/local/src/headers-more-nginx-module --add-module=/usr/local/src/echo-nginx-module --add-module=/usr/local/src/ngx_http_substitutions_filter_module --add-module=/usr/local/src/srcache-nginx-module --add-module=/usr/local/src/redis2-nginx-module --add-module=/usr/local/src/ngx_http_redis-0.3.8 --add-module=/usr/local/src/ngx_devel_kit --add-module=/usr/local/src/set-misc-nginx-module --add-module=/usr/local/src/nginx-module-vts --with-cc-opt='-DTCP_FASTOPEN=23 -march=native -flto -O3 -fuse-linker-plugin -Wno-error=strict-aliasing -fstack-protector-strong -D_FORTIFY_SOURCE=2' --with-ld-opt='-lrt -z relro -fstack-protector-strong'

@vkrasnov
Copy link
Contributor

I will take a look at it

@HansVanEijsden
Copy link

@vkrasnov thanks, let me know if you need more info (I can reproduce it on all of my servers).

@HansVanEijsden
Copy link

@vkrasnov are you able to reproduce the issue? Is there anything I can help you with, to make the patch working again?
@centminmod Did you have testing-time for it already (I'm following https://community.centminmod.com/threads/nginx-announce-nginx-1-13-6.13079/) too?

@vkrasnov
Copy link
Contributor

vkrasnov commented Oct 17, 2017

@HansVanEijsden, no. Can you try removing:

    if (h2c->table_update) {
        ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0,
                       "http2 table size update: 0");
        *pos++ = (1 << 5) | 0;
        h2c->table_update = 0;
    }

from ngx_http_v2_filter_module.c ?
It conflicts with the patch logic.

@HansVanEijsden
Copy link

Thanks for the update @vkrasnov - I've followed your advice and removed exactly those lines from ngx_http_v2_filter_module.c and it gives me these errors when building now:

cc -c -pipe  -O -W -Wall -Wpointer-arith -Wno-unused-parameter -Werror -g -DTCP_FASTOPEN=23 -march=native -flto -O3 -fuse-linker-plugin -Wno-error=strict-aliasing -fstack-protector-strong -D_FORTIFY_SOURCE=2  -D_GLIBCXX_USE_CXX11_ABI=0 -DNDK_SET_VAR -DNDK_UPSTREAM_LIST -I src/core -I src/event -I src/event/modules -I src/os/unix -I /usr/local/src/ngx_brotli_module/brotli/include/ -I /usr/local/src/ngx_devel_kit/objs -I objs/addon/ndk -I /usr/local/src/openssl-1.1.0f/.openssl/include -I objs -I src/http -I src/http/modules -I src/http/v2 -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/chromium/src -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/google-sparsehash/src/src -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/google-sparsehash/gen/arch/linux/x64/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/grpc/src/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/protobuf/src/src -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/re2/src -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/out/Release/obj/gen -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/out/Release/obj/gen/protoc_out/instaweb -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/apr/src/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/aprutil/src/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/apr/gen/arch/linux/x64/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/third_party/aprutil/gen/arch/linux/x64/include -I /home/hans/ngx_pagespeed-1.12.34.3-stable/psol/include/url -I /usr/local/src/ngx_devel_kit/src -I /usr/local/src/ngx_devel_kit/src -I /usr/local/src/ngx_devel_kit/objs -I objs/addon/ndk \
	-o objs/src/http/modules/ngx_http_gzip_filter_module.o \
	src/http/modules/ngx_http_gzip_filter_module.c
src/http/v2/ngx_http_v2_filter_module.c: In function ‘ngx_http_v2_header_filter’:
src/http/v2/ngx_http_v2_filter_module.c:132:32: error: unused variable ‘frame’ [-Werror=unused-variable]
     ngx_http_v2_out_frame_t   *frame;
                                ^~~~~
src/http/v2/ngx_http_v2_filter_module.c:131:32: error: unused variable ‘cln’ [-Werror=unused-variable]
     ngx_http_cleanup_t        *cln;
                                ^~~
src/http/v2/ngx_http_v2_filter_module.c:124:47: error: unused variable ‘start’ [-Werror=unused-variable]
     u_char                     status, *pos, *start, *p, *tmp;
                                               ^~~~~
src/http/v2/ngx_http_v2_filter_module.c: At top level:
src/http/v2/ngx_http_v2_filter_module.c:421:5: error: data definition has no type or storage class [-Werror]
     h2c = r->stream->connection;
     ^~~
src/http/v2/ngx_http_v2_filter_module.c:421:5: error: type defaults to ‘int’ in declaration of ‘h2c’ [-Werror=implicit-int]
src/http/v2/ngx_http_v2_filter_module.c:421:11: error: ‘r’ undeclared here (not in a function)
     h2c = r->stream->connection;
           ^
src/http/v2/ngx_http_v2_filter_module.c:423:5: error: expected identifier or ‘(’ before ‘if’
     if (h2c->indicate_resize) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:437:5: error: expected identifier or ‘(’ before ‘if’
     if (status) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:440:7: error: expected identifier or ‘(’ before ‘else’
     } else {
       ^~~~
src/http/v2/ngx_http_v2_filter_module.c:446:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.server == NULL) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:458:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.date == NULL) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:462:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.content_type.len) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:492:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.content_length == NULL
     ^~
src/http/v2/ngx_http_v2_filter_module.c:501:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.last_modified == NULL
     ^~
src/http/v2/ngx_http_v2_filter_module.c:511:5: error: expected identifier or ‘(’ before ‘if’
     if (r->headers_out.location && r->headers_out.location->value.len) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:516:5: error: expected identifier or ‘(’ before ‘if’
     if (r->gzip_vary) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:521:5: error: data definition has no type or storage class [-Werror]
     part = &r->headers_out.headers.part;
     ^~~~
src/http/v2/ngx_http_v2_filter_module.c:521:5: error: type defaults to ‘int’ in declaration of ‘part’ [-Werror=implicit-int]
src/http/v2/ngx_http_v2_filter_module.c:522:5: error: data definition has no type or storage class [-Werror]
     header = part->elts;
     ^~~~~~
src/http/v2/ngx_http_v2_filter_module.c:522:5: error: type defaults to ‘int’ in declaration of ‘header’ [-Werror=implicit-int]
src/http/v2/ngx_http_v2_filter_module.c:522:18: error: invalid type argument of ‘->’ (have ‘int’)
     header = part->elts;
                  ^~
src/http/v2/ngx_http_v2_filter_module.c:524:5: error: expected identifier or ‘(’ before ‘for’
     for (i = 0; /* void */; i++) {
     ^~~
src/http/v2/ngx_http_v2_filter_module.c:524:30: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘++’ token
     for (i = 0; /* void */; i++) {
                              ^~
src/http/v2/ngx_http_v2_filter_module.c:546:5: error: data definition has no type or storage class [-Werror]
     frame = ngx_http_v2_create_headers_frame(r, start, pos, r->header_only);
     ^~~~~
src/http/v2/ngx_http_v2_filter_module.c:546:5: error: type defaults to ‘int’ in declaration of ‘frame’ [-Werror=implicit-int]
src/http/v2/ngx_http_v2_filter_module.c:546:49: error: ‘start’ undeclared here (not in a function)
     frame = ngx_http_v2_create_headers_frame(r, start, pos, r->header_only);
                                                 ^~~~~
src/http/v2/ngx_http_v2_filter_module.c:546:56: error: ‘pos’ undeclared here (not in a function)
     frame = ngx_http_v2_create_headers_frame(r, start, pos, r->header_only);
                                                        ^~~
src/http/v2/ngx_http_v2_filter_module.c:547:5: error: expected identifier or ‘(’ before ‘if’
     if (frame == NULL) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:551:38: error: expected ‘)’ before ‘->’ token
     ngx_http_v2_queue_blocked_frame(r->stream->connection, frame);
                                      ^~
src/http/v2/ngx_http_v2_filter_module.c:553:6: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
     r->stream->queued = 1;
      ^~
src/http/v2/ngx_http_v2_filter_module.c:555:5: error: data definition has no type or storage class [-Werror]
     cln = ngx_http_cleanup_add(r, 0);
     ^~~
src/http/v2/ngx_http_v2_filter_module.c:555:5: error: type defaults to ‘int’ in declaration of ‘cln’ [-Werror=implicit-int]
src/http/v2/ngx_http_v2_filter_module.c:556:5: error: expected identifier or ‘(’ before ‘if’
     if (cln == NULL) {
     ^~
src/http/v2/ngx_http_v2_filter_module.c:560:8: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
     cln->handler = ngx_http_v2_filter_cleanup;
        ^~
src/http/v2/ngx_http_v2_filter_module.c:561:8: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
     cln->data = r->stream;
        ^~
src/http/v2/ngx_http_v2_filter_module.c:563:7: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
     fc->send_chain = ngx_http_v2_send_chain;
       ^~
src/http/v2/ngx_http_v2_filter_module.c:564:7: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
     fc->need_last_buf = 1;
       ^~
src/http/v2/ngx_http_v2_filter_module.c:566:5: error: expected identifier or ‘(’ before ‘return’
     return ngx_http_v2_filter_send(fc, r->stream);
     ^~~~~~
src/http/v2/ngx_http_v2_filter_module.c:567:1: error: expected identifier or ‘(’ before ‘}’ token
 }
 ^
src/http/v2/ngx_http_v2_filter_module.c: In function ‘ngx_http_v2_header_filter’:
src/http/v2/ngx_http_v2_filter_module.c:419:5: error: control reaches end of non-void function [-Werror=return-type]
     }
     ^
At top level:
src/http/v2/ngx_http_v2_filter_module.c:1435:1: error: ‘ngx_http_v2_filter_cleanup’ defined but not used [-Werror=unused-function]
 ngx_http_v2_filter_cleanup(void *data)
 ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/http/v2/ngx_http_v2_filter_module.c:836:1: error: ‘ngx_http_v2_send_chain’ defined but not used [-Werror=unused-function]
 ngx_http_v2_send_chain(ngx_connection_t *fc, ngx_chain_t *in, off_t limit)
 ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
objs/Makefile:1221: recept voor doel 'objs/src/http/v2/ngx_http_v2_filter_module.o' is mislukt
make[1]: *** [objs/src/http/v2/ngx_http_v2_filter_module.o] Fout 1
make[1]: *** Wachten op onvoltooide taken...
make[1]: Map '/usr/local/src/nginx-1.13.6' wordt verlaten
Makefile:11: recept voor doel 'install' is mislukt
make: *** [install] Fout 2

@vkrasnov
Copy link
Contributor

Probably cause it is missing the closing }. You should remove it too.

@HansVanEijsden
Copy link

My original ngx_http_v2_filter_module.c looks like this:

    start = pos;

    if (h2c->table_update) {
        ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0,
                       "http2 table size update: 0");
        *pos++ = (1 << 5) | 0;
        h2c->table_update = 0;
    }

According to your example I assume I should leave h2c->table_update = 0; there?

@vkrasnov
Copy link
Contributor

It doesn't matter, as long as *pos++ = (1 << 5) | 0; is not there. I just need to know if that is the problem.

@vkrasnov
Copy link
Contributor

I corrected the code to be removed. The start = pos should stay.

@HansVanEijsden
Copy link

I confirm to have it working @vkrasnov, after removing those lines.
No more errors (yet), and HPACK works great again too. Thanks for the effort! 👍🏻

$ h2load https://www.weblogzwolle.nl -n 100 | tail -6 |head -1
traffic: 7.94MB (8329738) total, 2.04KB (2089) headers (space savings 97.40%), 7.93MB (8315900) data

@ahmed-sigmalux
Copy link

Any plans to make this fix in the master branch?

@vkrasnov
Copy link
Contributor

vkrasnov commented Nov 6, 2017

Working on it.

@kevin25
Copy link

kevin25 commented Nov 9, 2017

Any update on this guys?

objs/Makefile:983: recipe for target 'objs/src/http/v2/ngx_http_v2_filter_module.o' failed
make[1]: *** [objs/src/http/v2/ngx_http_v2_filter_module.o] Error 1
make[1]: Leaving directory '/usr/local/src/nginx-1.13.6'
Makefile:8: recipe for target 'build' failed
make: *** [build] Error 2

@IThinkIGottaGo
Copy link

IThinkIGottaGo commented Jan 9, 2018

I search google very hard finally find here....And thanks to friends above work to this problem. I am using Nginx 1.13.8 and today was Jan 9th ,met same problem. Because my English not very well ,so at began I didn't figure out HansVanEijsden which code that he deleted. But finally i found it. Actually it was a duplicated variable name error. So for convenient if anyone also met same thing and came here before the master branch solve this problem, There is a easy way to solve the problem here.

First, if you came here you must already patched "HPACK 1.13.6". So get into your nginx dir, and move to XXX/nginx-1.13.8/src/http/v2 dir. You will find there will be a file named ngx_http_v2_filter_module.c. And use vim or other method open and edit it. Then find line number #122 and check the function named ngx_http_v2_header_filter. Then you will find there will be two same name variable named ngx_http_v2_connection_t *h2c; in the line #133 and #137. Finally delete anyone and save it. Back to your XXX/nginx-1.13.8/ dir and continue make it. if there wasn't others problem, everything should be OK now. Hope my poor grammar didn't affect you try to fix the program. : )

@jarredou
Copy link

@FreeStyleNight Thanks for the simple recap/update of the thread ! It solved my problem !

@PikachuEXE
Copy link

Created a PR for 1.13.9

@kn007
Copy link

kn007 commented Mar 14, 2018

For nginx 1.13.9, could using this patch: https://github.com/kn007/patch/blob/master/nginx.patch

@injust
Copy link
Contributor

injust commented Mar 14, 2018

This (standalone HPACK patch) should work for nginx 1.13.9.

@PikachuEXE
Copy link

Update: I am using #92 now
Fix code injection position for usage of indicate_resize

@centminmod
Copy link
Author

centminmod commented Mar 20, 2018

FYI, 1.3.10 will also break this hpack patch #93

@kevin25
Copy link

kevin25 commented Aug 17, 2019

Is there any update for Nginx 1.16.1?

@PikachuEXE
Copy link

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