http2: convert an assert to run-time check #2514
Conversation
Fuzzing has proven we can reach code in on_frame_recv with status_code not having been set, so let's detect that in run-time (instead of with assert) and error error accordingly. Detected by OSS-Fuzz Bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7903
|
Still present with master nghttp2 (as opposed to 1.24). @tatsuhiro-t: does this assert indicate a potential problem in nghttp2? |
|
I have no permission to view the page, but it looks like https://github.com/curl/curl/pull/2514/files#diff-7dcf04be672466b7a56e6a81df098c6bR635 is suspicious to me. I think we should set -1 to status_code only for 1xx status code. |
|
For reference, the relevant http2 data we're receiving in our error case is in this pcap file: http2_5380578661629952.zip |
|
@tatsuhiro-t: looking at this a bit more; it looks like status_code is only being set in I don't have any more knowledge here about whether the status_code comment is correct or not; however, I think this fix is likely to solve our problems by basically being defensive. |
|
It looks like something is wrong in nghttp2 code. Will look into it. |
|
I identified bug in nghttp2, and fixed it now. |
|
Awesome @tatsuhiro-t, its great to see that our fuzzing indirectly helped you as well! =) So, with that new find and fix of yours in mind, do you think my PR here then makes sense for curl when using older libnghttp2 installations? |
|
@bagder Yes, I do. It is better than failing with assertion error. And this PR works as well with latest nghttp2 code. |
Fuzzing has proven we can reach code in on_frame_recv with status_code
not having been set, so let's detect that in run-time (instead of with
assert) and error error accordingly.
Detected by OSS-Fuzz
Bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7903