envoyproxy / envoyproxy/envoy

Is onInvalidFrame() method in http2/codec_impl.cc preventing GO_AWAY frame from being sent for protocol errors?

Open
#11,752 1 comment 0 reactions 0 assignees View on GitHub
area/http bug help wanted
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 20h
Merged PRs (30d)
428

Description

### Description:

I have a case where I am not receiving a GO_AWAY frame when sending an incorrect stream_id in the HEADER frame. From what I can tell, the `session_handle_invalid_connection()` method inside the `nghttp2_session` class is invoking the `onInvalidFrame()` callback inside the `http2/codec_impl.cc` class with `NGHTTP2_ERR_PROTO` as the error code. However, since the `onInvalidFrame()` method is not returning 0, the GO_AWAY frame is never sent. Is this normal?

### session_handle_invalid_connection() method:
```
static int session_handle_invalid_connection(nghttp2_session *session,
nghttp2_frame *frame,
int lib_error_code,
const char *reason) {
if (session->callbacks.on_invalid_frame_recv_callback) {
if (session->callbacks.on_invalid_frame_recv_callback(
session, frame, lib_error_code, session->user_data) != 0) {
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
}
return nghttp2_session_terminate_session_with_reason(
session, get_error_code_from_lib_error_code(lib_error_code), reason);
}
```

### onInvalidFrame() callback:
```
int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) {
ENVOY_CONN_LOG(debug, "invalid frame: {} on stream {}", connection_, nghttp2_strerror(error_code),
stream_id);

if (error_code == NGHTTP2_ERR_HTTP_HEADER || error_code == NGHTTP2_ERR_HTTP_MESSAGING) {
stats_.rx_messaging_error_.inc();

if (stream_error_on_invalid_http_messaging_) {
// The stream is about to be closed due to an invalid header or messaging. Don't kill the
// entire connection if one stream has bad headers or messaging.
StreamImpl* stream = getStream(stream_id);
if (stream != nullptr) {
// See comment below in onStreamClose() for why we do this.
stream->reset_due_to_messaging_error_ = true;
}
return 0;
}
}

// Cause dispatch to return with an error code.
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
```

### Snippet of backtrace:

> #0 Envoy::Http::Http2::ConnectionImpl::onInvalidFrame (this=0x55555a0329b8, stream_id=2, error_code=-505) at external/envoy/source/common/http/http2/codec_impl.cc:636
> #1 0x0000555556a43149 in Envoy::Http::Http2::ConnectionImpl::Http2Callbacks::::operator()(nghttp2_session *, const nghttp2_frame *, int, void *) const (__closure=0x0, frame=0x555559d7d3a0, error_code=-505, user_data=0x55555a0329b8) at external/envoy/source/common/http/http2/codec_impl.cc:1009
> #2 0x0000555556a43181 in Envoy::Http::Http2::ConnectionImpl::Http2Callbacks::::_FUN(nghttp2_session *, const nghttp2_frame *, int, void *) ()
> at external/envoy/source/common/http/http2/codec_impl.cc:1010
> #3 0x0000555556a5e339 in session_handle_invalid_connection (session=0x555559d7d200, frame=0x555559d7d3a0, lib_error_code=-505, reason=0x555557dcbed0 "request HEADERS: client received request")
> at /home/ekishpa/.cache/bazel/_bazel_ekishpa/85e9f933f608b792c07cb9f0a32a9492/sandbox/linux-sandbox/1/execroot/adc/external/com_github_nghttp2_nghttp2/lib/nghttp2_session.c:3518
> #4 0x0000555556a5e392 in session_inflate_handle_invalid_connection (session=0x555559d7d200, frame=0x555559d7d3a0, lib_error_code=-505, reason=0x555557dcbed0 "request HEADERS: client received request")
> at /home/ekishpa/.cache/bazel/_bazel_ekishpa/85e9f933f608b792c07cb9f0a32a9492/sandbox/linux-sandbox/1/execroot/adc/external/com_github_nghttp2_nghttp2/lib/nghttp2_session.c:3532
> #5 0x0000555556a5ec51 in nghttp2_session_on_request_headers_received (session=0x555559d7d200, frame=0x555559d7d3a0)
> at /home/ekishpa/.cache/bazel/_bazel_ekishpa/85e9f933f608b792c07cb9f0a32a9492/sandbox/linux-sandbox/1/execroot/adc/external/com_github_nghttp2_nghttp2/lib/nghttp2_session.c:3833
> #6 0x0000555556a5f2e8 in session_process_headers_frame (session=0x555559d7d200)
> at /home/ekishpa/.cache/bazel/_bazel_ekishpa/85e9f933f608b792c07cb9f0a32a9492/sandbox/linux-sandbox/1/execroot/adc/external/com_github_nghttp2_nghttp2/lib/nghttp2_session.c:4060
> #7 0x0000555556a62015 in nghttp2_session_mem_recv (session=0x555559d7d200,
> in=0x55555a07f03a "\210@\fcontent-type\020application/grpc@\024grpc-accept-encoding\025identity,deflate,gzip@\017accept-encoding\ridentity,gzip", inlen=487)
> at /home/ekishpa/.cache/bazel/_bazel_ekishpa/85e9f933f608b792c07cb9f0a32a9492/sandbox/linux-sandbox/1/execroot/adc/external/com_github_nghttp2_nghttp2/lib/nghttp2_session.c:5595
> #8 0x0000555556a3f3d9 in Envoy::Http::Http2::ConnectionImpl::dispatch (this=0x55555a0329b8, data=...) at external/envoy/source/common/http/http2/codec_impl.cc:412

Contributor guide

Open the contributing guide

Research direction

Start by reading external/envoy/source/common/http/http2/codec_impl.cc, especially ConnectionImpl::onInvalidFrame and the callback setup around the reported lines. Trace the call into nghttp2_session_mem_recv and session_handle_invalid_connection, then reproduce the incorrect stream_id case to observe whether a GO_AWAY frame is queued. Done means the expected protocol-error behavior is established and covered by an appropriate regression test.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.