From f58d422123b6565ea3fdc529607842f007611300 Mon Sep 17 00:00:00 2001 From: Matthew Douglass <5410142+mdouglass@users.noreply.github.com> Date: Fri, 18 Jun 2021 10:01:52 -0700 Subject: [PATCH] http2: Do not call non-reentrant nghttp2 functions from nghttp2 callbacks The nghttp2 functions nghttp2_session_send, nghttp2_session_mem_send, nghttp2_session_recv and nghttp2_session_mem_recv are documented as not being able to be called from nghttp2 callback functions. Add a tracker so we can tell when we are within the scope of an nghttp2 callback and early return from ConsumeHTTP2Data and SendPendingData which are the only two methods that use the prohibited methods. This prevents a use-after-free crash bug that can be triggered otherwise. Fixes: https://github.com/nodejs/node/issues/38964 Refs: https://nghttp2.org/documentation/programmers-guide.html#remarks --- src/node_http2.cc | 39 ++++++++++++++++++++++++++++++++++++++- src/node_http2.h | 4 ++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d11bcf5eac077f..a434495feef19f 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -54,6 +54,18 @@ bool HasHttp2Observer(Environment* env) { } // anonymous namespace +class NgHttp2CallbackScope { +public: + explicit NgHttp2CallbackScope(Http2Session* session) : session_(session) { + ++session_->nghttp2_callback_scope_; + } + ~NgHttp2CallbackScope() { + --session_->nghttp2_callback_scope_; + } +private: + BaseObjectPtr session_; +}; + // These configure the callbacks required by nghttp2 itself. There are // two sets of callback functions, one that is used if a padding callback // is set, and other that does not include the padding callback. @@ -788,6 +800,12 @@ void Http2Session::ConsumeHTTP2Data() { CHECK_LE(stream_buf_offset_, stream_buf_.len); size_t read_len = stream_buf_.len - stream_buf_offset_; + // ConsumeHTTP2Data should not be called from within an nghttp2 callback + if (nghttp2_callback_scope_ > 0) { + Debug(this, "skipping receiving from within nghttp2 callback"); + return; + } + // multiple side effects. Debug(this, "receiving %d bytes [wants data? %d]", read_len, @@ -869,6 +887,7 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, const nghttp2_frame* frame, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); int32_t id = GetFrameID(frame); Debug(session, "beginning headers for stream %d", id); @@ -908,6 +927,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, uint8_t flags, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); int32_t id = GetFrameID(frame); BaseObjectPtr stream = session->FindStream(id); // If stream is null at this point, either something odd has happened @@ -933,6 +953,7 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle, const nghttp2_frame* frame, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); session->statistics_.frame_count++; Debug(session, "complete frame received: type: %d", frame->hd.type); @@ -973,6 +994,7 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, int lib_error_code, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); const uint32_t max_invalid_frames = session->js_fields_->max_invalid_frames; Debug(session, @@ -1010,6 +1032,7 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, int error_code, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); Environment* env = session->env(); Debug(session, "frame type %d was not sent, code: %d", frame->hd.type, error_code); @@ -1042,6 +1065,7 @@ int Http2Session::OnFrameSent(nghttp2_session* handle, const nghttp2_frame* frame, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); session->statistics_.frame_sent += 1; return 0; } @@ -1052,6 +1076,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, uint32_t code, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); Environment* env = session->env(); Isolate* isolate = env->isolate(); HandleScope scope(isolate); @@ -1084,13 +1109,15 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, // ignore these. If this callback was not provided, nghttp2 would handle // invalid headers strictly and would shut down the stream. We are intentionally // being more lenient here although we may want to revisit this choice later. -int Http2Session::OnInvalidHeader(nghttp2_session* session, +int Http2Session::OnInvalidHeader(nghttp2_session* handle, const nghttp2_frame* frame, nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags, void* user_data) { // Ignore invalid header fields by default. + Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); return 0; } @@ -1105,6 +1132,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, size_t len, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); Debug(session, "buffering data chunk for stream %d, size: " "%d, flags: %d", id, len, flags); Environment* env = session->env(); @@ -1185,6 +1213,7 @@ ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, size_t maxPayloadLen, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); ssize_t padding = frame->hd.length; switch (session->padding_strategy_) { @@ -1214,6 +1243,7 @@ int Http2Session::OnNghttpError(nghttp2_session* handle, // Unfortunately, this is currently the only way for us to know if // the session errored because the peer is not an http2 peer. Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); Debug(session, "Error '%s'", message); if (strncmp(message, BAD_PEER_MESSAGE, len) == 0) { Environment* env = session->env(); @@ -1676,6 +1706,12 @@ uint8_t Http2Session::SendPendingData() { // This is cleared by ClearOutgoing(). set_sending(); + // SendPendingData should not be called from within an nghttp2 callback + if (nghttp2_callback_scope_ > 0) { + Debug(this, "skipping sending pending data from within nghttp2 callback"); + return 1; + } + ssize_t src_length; const uint8_t* src; @@ -1754,6 +1790,7 @@ int Http2Session::OnSendData( nghttp2_data_source* source, void* user_data) { Http2Session* session = static_cast(user_data); + NgHttp2CallbackScope ngcbScope(session); BaseObjectPtr stream = session->FindStream(frame->hd.stream_id); if (!stream) return 0; diff --git a/src/node_http2.h b/src/node_http2.h index 4d267e647d3494..c823b2b9b140f8 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -921,6 +921,9 @@ class Http2Session : public AsyncWrap, // Also use the invalid frame count as a measure for rejecting input frames. uint32_t invalid_frame_count_ = 0; + // keep track of the depth of nghttp2 callbacks + uint32_t nghttp2_callback_scope_ = 0; + void PushOutgoingBuffer(NgHttp2StreamWrite&& write); BaseObjectPtr http2_state_; @@ -930,6 +933,7 @@ class Http2Session : public AsyncWrap, friend class Http2Scope; friend class Http2StreamListener; + friend class NgHttp2CallbackScope; }; struct Http2SessionPerformanceEntryTraits {