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

Rework HTTP/2 closed connection logic and consolidate http2 mem_recv() calls in one place. #5648

Closed
wants to merge 5 commits into from

Conversation

@laramiel
Copy link
Contributor

laramiel commented Jul 3, 2020

Previously there were several locations that called
nghttp2_session_mem_recv and handled responses slightly differently.
Those have been converted to call the existing
h2_process_pending_input() function.

Moved the end-of-session check to h2_process_pending_input() since
the only place the end-of-session state can change is after nghttp2
processes additional input frames.

This will likely fix the fuzzing error. While I don't have a root
cause the out-of-bounds read seems like a use after free, so moving
the nghttp2_session_check_request_allowed() call to a location with
a guaranteed nghttp2 session seems reasonable.

Also updated a few nghttp2 callsites to include error messages and
added a few additional error checks.

This attempts to address #5646

@laramiel laramiel force-pushed the laramiel:master branch from 9825e22 to 5656351 Jul 3, 2020
Previously there were several locations that called
nghttp2_session_mem_recv and handled responses slightly differently.
Those have been converted to call the existing
h2_process_pending_input() function.

Moved the end-of-session check to h2_process_pending_input() since
the only place the end-of-session state can change is after nghttp2
processes additional input frames.

This will likely fix the fuzzing error. While I don't have a root
cause the out-of-bounds read seems like a use after free, so moving
the nghttp2_session_check_request_allowed() call to a location with
a guaranteed nghttp2 session seems reasonable.

Also updated a few nghttp2 callsites to include error messages and
added a few additional error checks.
@laramiel laramiel force-pushed the laramiel:master branch from 1ce001c to 3ed164b Jul 3, 2020
During merge left in the old connclose call. Remove.
Also fixup style: if (  =>  if(
@bagder
Copy link
Member

bagder commented Jul 3, 2020

http2 warnings:

http2.c: In function ‘http2_recv’:
http2.c:1659:11: error: variable ‘inbuf’ set but not used [-Werror=unused-but-set-variable]
     char *inbuf;
           ^~~~~
http2.c:1538:11: error: unused variable ‘rv’ [-Werror=unused-variable]
   ssize_t rv;
           ^~
http2.c: In function ‘Curl_http2_switched’:
http2.c:2178:11: error: unused variable ‘nproc’ [-Werror=unused-variable]
   ssize_t nproc;
           ^~~~~
cc1: all warnings being treated as errors
@bagder
Copy link
Member

bagder commented Jul 3, 2020

checksrc warnings:

./http2.c:1334:13: warning: space before close parenthesis (SPACEBEFORECLOSE)
   if(rv < 0 ) {
             ^
./http2.c:1695:85: warning: Longer than 79 columns (LONGLINE)
       H2BUGF(infof(data, "Use data left in connection buffer, nread=%zd\n", nread));
./http2.c:2052:58: warning: no space before asterisk (ASTERISKNOSPACE)
          nghttp2_session_check_request_allowed(h2), (void*)conn->data));
                                                          ^
./http2.c:2078:83: warning: Longer than 79 columns (LONGLINE)
     H2BUGF(infof(conn->data, "http2_send() nghttp2_submit_request error (%s)%d\n",
./http2.c:2092:81: warning: Longer than 79 columns (LONGLINE)
     H2BUGF(infof(conn->data, "http2_send() nghttp2_session_send error (%s)%d\n",
checksrc: 0 errors and 5 warnings
checksrc: 0 errors and 6 warnings suppressed
Makefile:4260: recipe for target 'checksrc' failed
@jay jay added the HTTP/2 label Jul 6, 2020
@laramiel
Copy link
Contributor Author

laramiel commented Jul 8, 2020

Ping on this. I can move the close connection back, but I think that it makes sense to consolidate the receive logic into a single method.

@bagder
Copy link
Member

bagder commented Jul 9, 2020

I'm slow this week, I'll review/merge this soon.

@bagder bagder closed this in 25a25f4 Sep 10, 2020
@bagder
Copy link
Member

bagder commented Sep 10, 2020

Thanks @laramiel, sorry for taking forever to merge this!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.