Skip to content

Commit

Permalink
http2: Do not call non-reentrant nghttp2 functions from nghttp2 callb…
Browse files Browse the repository at this point in the history 
…acks

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: nodejs#38964
Refs: https://nghttp2.org/documentation/programmers-guide.html#remarks
  • Loading branch information
@mdouglass
mdouglass committed Jun 18, 2021
1 parent 3f4b0a3 commit f58d422
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
39 changes: 38 additions & 1 deletion src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http2Session> 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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -869,6 +887,7 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle,
const nghttp2_frame* frame,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
int32_t id = GetFrameID(frame);
Debug(session, "beginning headers for stream %d", id);

Expand Down Expand Up @@ -908,6 +927,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle,
uint8_t flags,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
int32_t id = GetFrameID(frame);
BaseObjectPtr<Http2Stream> stream = session->FindStream(id);
// If stream is null at this point, either something odd has happened
Expand All @@ -933,6 +953,7 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle,
const nghttp2_frame* frame,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
session->statistics_.frame_count++;
Debug(session, "complete frame received: type: %d",
frame->hd.type);
Expand Down Expand Up @@ -973,6 +994,7 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
int lib_error_code,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
const uint32_t max_invalid_frames = session->js_fields_->max_invalid_frames;

Debug(session,
Expand Down Expand Up @@ -1010,6 +1032,7 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
int error_code,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
Environment* env = session->env();
Debug(session, "frame type %d was not sent, code: %d",
frame->hd.type, error_code);
Expand Down Expand Up @@ -1042,6 +1065,7 @@ int Http2Session::OnFrameSent(nghttp2_session* handle,
const nghttp2_frame* frame,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
session->statistics_.frame_sent += 1;
return 0;
}
Expand All @@ -1052,6 +1076,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
uint32_t code,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
Environment* env = session->env();
Isolate* isolate = env->isolate();
HandleScope scope(isolate);
Expand Down Expand Up @@ -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<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
return 0;
}

Expand All @@ -1105,6 +1132,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
size_t len,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
Debug(session, "buffering data chunk for stream %d, size: "
"%d, flags: %d", id, len, flags);
Environment* env = session->env();
Expand Down Expand Up @@ -1185,6 +1213,7 @@ ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle,
size_t maxPayloadLen,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
ssize_t padding = frame->hd.length;

switch (session->padding_strategy_) {
Expand Down Expand Up @@ -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<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
Debug(session, "Error '%s'", message);
if (strncmp(message, BAD_PEER_MESSAGE, len) == 0) {
Environment* env = session->env();
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -1754,6 +1790,7 @@ int Http2Session::OnSendData(
nghttp2_data_source* source,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
NgHttp2CallbackScope ngcbScope(session);
BaseObjectPtr<Http2Stream> stream = session->FindStream(frame->hd.stream_id);
if (!stream) return 0;

Expand Down
4 changes: 4 additions & 0 deletions src/node_http2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http2State> http2_state_;
Expand All @@ -930,6 +933,7 @@ class Http2Session : public AsyncWrap,

friend class Http2Scope;
friend class Http2StreamListener;
friend class NgHttp2CallbackScope;
};

struct Http2SessionPerformanceEntryTraits {
Expand Down

0 comments on commit f58d422

Please sign in to comment.