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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

laramiel
Copy link
Contributor

@laramiel laramiel commented Jul 3, 2020 •
edited

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 mentioned this pull request Jul 3, 2020
Closed
@laramiel
Review nghttp2_session_mem_recv() call sites and consolidate.
3ed164b 
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
Fixup merge & style.
2cfcf68 
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

@bagder bagder mentioned this pull request Jul 3, 2020
Closed
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
HTTP/2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@laramiel @bagder @jay