Skip to content

Commit

Permalink
http2: add specific error code for custom frames
Browse files Browse the repository at this point in the history
As suggested in
#37849 (comment)
improve the error presented when encountering a large number of
invalid frames by giving this situation a specific error code (which we
should have had from the beginning).

PR-URL: #37936
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Apr 4, 2021
1 parent 51e7a33 commit 062541a
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 31 deletions.
9 changes: 9 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,15 @@ When setting the priority for an HTTP/2 stream, the stream may be marked as
a dependency for a parent stream. This error code is used when an attempt is
made to mark a stream and dependent of itself.

<a id="ERR_HTTP2_TOO_MANY_INVALID_FRAMES"></a>
### `ERR_HTTP2_TOO_MANY_INVALID_FRAMES`
<!--
added: REPLACEME
-->

The limit of acceptable invalid HTTP/2 protocol frames sent by the peer,
as specified through the `maxSessionInvalidFrames` option, has been exceeded.

<a id="ERR_HTTP2_TRAILERS_ALREADY_SENT"></a>
### `ERR_HTTP2_TRAILERS_ALREADY_SENT`

Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,7 @@ E('ERR_HTTP2_STREAM_CANCEL', function(error) {
E('ERR_HTTP2_STREAM_ERROR', 'Stream closed with error code %s', Error);
E('ERR_HTTP2_STREAM_SELF_DEPENDENCY',
'A stream cannot depend on itself', Error);
E('ERR_HTTP2_TOO_MANY_INVALID_FRAMES', 'Too many invalid HTTP/2 frames', Error);
E('ERR_HTTP2_TRAILERS_ALREADY_SENT',
'Trailing headers have already been sent', Error);
E('ERR_HTTP2_TRAILERS_NOT_READY',
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -777,9 +777,9 @@ const setAndValidatePriorityOptions = hideStackFrames((options) => {

// When an error occurs internally at the binding level, immediately
// destroy the session.
function onSessionInternalError(code) {
function onSessionInternalError(integerCode, customErrorCode) {
if (this[kOwner] !== undefined)
this[kOwner].destroy(new NghttpError(code));
this[kOwner].destroy(new NghttpError(integerCode, customErrorCode));
}

function settingsCallback(cb, ack, duration) {
Expand Down
13 changes: 8 additions & 5 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const {
ERR_INVALID_HTTP_TOKEN
},
addCodeToName,
getMessage,
hideStackFrames
} = require('internal/errors');

Expand Down Expand Up @@ -543,11 +544,13 @@ function mapToHeaders(map,
}

class NghttpError extends Error {
constructor(ret) {
super(binding.nghttp2ErrorString(ret));
this.code = 'ERR_HTTP2_ERROR';
this.errno = ret;
addCodeToName(this, super.name, 'ERR_HTTP2_ERROR');
constructor(integerCode, customErrorCode) {
super(customErrorCode ?
getMessage(customErrorCode, [], null) :
binding.nghttp2ErrorString(integerCode));
this.code = customErrorCode || 'ERR_HTTP2_ERROR';
this.errno = integerCode;
addCodeToName(this, super.name, this.code);
}

toString() {
Expand Down
58 changes: 38 additions & 20 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
Expand Down Expand Up @@ -732,7 +733,7 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen,
// various callback functions. Each of these will typically result in a call
// out to JavaScript so this particular function is rather hot and can be
// quite expensive. This is a potential performance optimization target later.
ssize_t Http2Session::ConsumeHTTP2Data() {
void Http2Session::ConsumeHTTP2Data() {
CHECK_NOT_NULL(stream_buf_.base);
CHECK_LE(stream_buf_offset_, stream_buf_.len);
size_t read_len = stream_buf_.len - stream_buf_offset_;
Expand All @@ -742,12 +743,14 @@ ssize_t Http2Session::ConsumeHTTP2Data() {
read_len,
nghttp2_session_want_read(session_.get()));
set_receive_paused(false);
custom_recv_error_code_ = nullptr;
ssize_t ret =
nghttp2_session_mem_recv(session_.get(),
reinterpret_cast<uint8_t*>(stream_buf_.base) +
stream_buf_offset_,
read_len);
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
CHECK_IMPLIES(custom_recv_error_code_ != nullptr, ret < 0);

if (is_receive_paused()) {
CHECK(is_reading_stopped());
Expand All @@ -759,7 +762,7 @@ ssize_t Http2Session::ConsumeHTTP2Data() {
// Even if all bytes were received, a paused stream may delay the
// nghttp2_on_frame_recv_callback which may have an END_STREAM flag.
stream_buf_offset_ += ret;
return ret;
goto done;
}

// We are done processing the current input chunk.
Expand All @@ -769,14 +772,34 @@ ssize_t Http2Session::ConsumeHTTP2Data() {
stream_buf_allocation_.clear();
stream_buf_ = uv_buf_init(nullptr, 0);

if (ret < 0)
return ret;

// Send any data that was queued up while processing the received data.
if (!is_destroyed()) {
if (ret >= 0 && !is_destroyed()) {
SendPendingData();
}
return ret;

done:
if (UNLIKELY(ret < 0)) {
Isolate* isolate = env()->isolate();
Debug(this,
"fatal error receiving data: %d (%s)",
ret,
custom_recv_error_code_ != nullptr ?
custom_recv_error_code_ : "(no custom error code)");
Local<Value> args[] = {
Integer::New(isolate, static_cast<int32_t>(ret)),
Null(isolate)
};
if (custom_recv_error_code_ != nullptr) {
args[1] = String::NewFromUtf8(
isolate,
custom_recv_error_code_,
NewStringType::kInternalized).ToLocalChecked();
}
MakeCallback(
env()->http2session_on_error_function(),
arraysize(args),
args);
}
}


Expand Down Expand Up @@ -900,14 +923,17 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
int lib_error_code,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
const uint32_t max_invalid_frames = session->js_fields_->max_invalid_frames;

Debug(session,
"invalid frame received (%u/%u), code: %d",
session->invalid_frame_count_,
session->js_fields_->max_invalid_frames,
max_invalid_frames,
lib_error_code);
if (session->invalid_frame_count_++ > session->js_fields_->max_invalid_frames)
if (session->invalid_frame_count_++ > max_invalid_frames) {
session->custom_recv_error_code_ = "ERR_HTTP2_TOO_MANY_INVALID_FRAMES";
return 1;
}

// If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
if (nghttp2_is_fatal(lib_error_code) ||
Expand Down Expand Up @@ -1286,6 +1312,7 @@ int Http2Session::HandleDataFrame(const nghttp2_frame* frame) {
stream->EmitRead(UV_EOF);
} else if (frame->hd.length == 0) {
if (invalid_frame_count_++ > js_fields_->max_invalid_frames) {
custom_recv_error_code_ = "ERR_HTTP2_TOO_MANY_INVALID_FRAMES";
Debug(this, "rejecting empty-frame-without-END_STREAM flood\n");
// Consider a flood of 0-length frames without END_STREAM an error.
return 1;
Expand Down Expand Up @@ -1470,7 +1497,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
ConsumeHTTP2Data();
}

if (!is_write_scheduled()) {
if (!is_write_scheduled() && !is_destroyed()) {
// Schedule a new write if nghttp2 wants to send data.
MaybeScheduleWrite();
}
Expand Down Expand Up @@ -1798,21 +1825,12 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
// offset of a DATA frame's data into the socket read buffer.
stream_buf_ = uv_buf_init(buf.data(), static_cast<unsigned int>(nread));

Isolate* isolate = env()->isolate();

// Store this so we can create an ArrayBuffer for read data from it.
// DATA frames will be emitted as slices of that ArrayBuffer to avoid having
// to copy memory.
stream_buf_allocation_ = std::move(buf);

ssize_t ret = ConsumeHTTP2Data();

if (UNLIKELY(ret < 0)) {
Debug(this, "fatal error receiving data: %d", ret);
Local<Value> arg = Integer::New(isolate, static_cast<int32_t>(ret));
MakeCallback(env()->http2session_on_error_function(), 1, &arg);
return;
}
ConsumeHTTP2Data();

MaybeStopReading();
}
Expand Down
8 changes: 6 additions & 2 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -660,8 +660,9 @@ class Http2Session : public AsyncWrap,
// Indicates whether there currently exist outgoing buffers for this stream.
bool HasWritesOnSocketForStream(Http2Stream* stream);

// Write data from stream_buf_ to the session
ssize_t ConsumeHTTP2Data();
// Write data from stream_buf_ to the session.
// This will call the error callback if an error occurs.
void ConsumeHTTP2Data();

void MemoryInfo(MemoryTracker* tracker) const override;
SET_MEMORY_INFO_NAME(Http2Session)
Expand Down Expand Up @@ -895,6 +896,9 @@ class Http2Session : public AsyncWrap,
v8::Global<v8::ArrayBuffer> stream_buf_ab_;
AllocatedBuffer stream_buf_allocation_;
size_t stream_buf_offset_ = 0;
// Custom error code for errors that originated inside one of the callbacks
// called by nghttp2_session_mem_recv.
const char* custom_recv_error_code_ = nullptr;

size_t max_outstanding_pings_ = kDefaultMaxPings;
std::queue<BaseObjectPtr<Http2Ping>> outstanding_pings_;
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-http2-empty-frame-without-eof.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ async function main() {
stream.on('error', common.mustNotCall());
client.on('error', common.mustNotCall());
} else {
stream.on('error', common.mustCall());
client.on('error', common.mustCall());
const expected = {
code: 'ERR_HTTP2_TOO_MANY_INVALID_FRAMES',
message: 'Too many invalid HTTP/2 frames'
};
stream.on('error', common.expectsError(expected));
client.on('error', common.expectsError(expected));
}
stream.resume();
await once(stream, 'end');
Expand Down

0 comments on commit 062541a

Please sign in to comment.