Skip to content

Commit 7784fa9

Browse files
tatsuhiro-ttatsuhiro-t
committed
Make error handling robust
Stream errors are now promoted to connection errors. This means that an event that previously just resets a single stream now closes a connection entirely. The promoted errors are mostly implementation errors. Some involve HTTP fields, but they are already treated stream error. People who care about that should have already raised any issues. We do not have any outstanding related issues now, so it seems OK to treat it as connection error. We have some contradictory specifications around nghttp2_on_invalid_header and nghttp2_on_invalid_header2 callbacks. nghttp2_on_invalid_header says that if it is omitted, a stream is reset. Meanwhile, nghttp2_on_invalid_header2 says that if it is omitted, invalid field is silently ignored. In actual implementation, if both omitted, we treat it as stream error. In practice, it is often required not to bail out if invalid header is received. In this change, if both callbacks are omitted, invalid field is silently ignored as the documentation of nghttp2_on_invalid_header2 says. The connection error promotion is applied here as well. So if invalid field is received, and callback returns NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE, it is treated as connection error.
1 parent 8391ae7 commit 7784fa9

5 files changed

Lines changed: 369 additions & 310 deletions

File tree

‎lib/includes/nghttp2/nghttp2.h‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2045,12 +2045,13 @@ typedef int (*nghttp2_on_header_callback2)(nghttp2_session *session,
20452045
* The parameter and behaviour are similar to
20462046
* :type:`nghttp2_on_header_callback`. The difference is that this
20472047
* callback is only invoked when a invalid header name/value pair is
2048-
* received which is treated as stream error if this callback is not
2049-
* set. Only invalid regular header field are passed to this
2050-
* callback. In other words, invalid pseudo header field is not
2051-
* passed to this callback. Also header fields which includes upper
2052-
* cased latter are also treated as error without passing them to this
2053-
* callback.
2048+
* received which is treated as stream error if this callback returns
2049+
* :enum:`nghttp2_error.NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE` and
2050+
* :type:`nghttp2_on_invalid_header_callback2` is not set. Only
2051+
* invalid regular header field are passed to this callback. In other
2052+
* words, invalid pseudo header field is not passed to this callback.
2053+
* Also header fields which includes upper cased latter are also
2054+
* treated as error without passing them to this callback.
20542055
*
20552056
* This callback is only considered if HTTP messaging validation is
20562057
* turned on (which is on by default, see
@@ -2082,11 +2083,12 @@ typedef int (*nghttp2_on_invalid_header_callback)(
20822083
* The parameter and behaviour are similar to
20832084
* :type:`nghttp2_on_header_callback2`. The difference is that this
20842085
* callback is only invoked when a invalid header name/value pair is
2085-
* received which is silently ignored if this callback is not set.
2086-
* Only invalid regular header field are passed to this callback. In
2087-
* other words, invalid pseudo header field is not passed to this
2088-
* callback. Also header fields which includes upper cased latter are
2089-
* also treated as error without passing them to this callback.
2086+
* received which is silently ignored if neither this callback nor
2087+
* :type:`nghttp2_on_invalid_header_callback` is set. Only invalid
2088+
* regular header field are passed to this callback. In other words,
2089+
* invalid pseudo header field is not passed to this callback. Also
2090+
* header fields which includes upper cased latter are also treated as
2091+
* error without passing them to this callback.
20902092
*
20912093
* This callback is only considered if HTTP messaging validation is
20922094
* turned on (which is on by default, see

‎lib/nghttp2_int.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,11 @@ typedef enum {
5252
* Unlike NGHTTP2_ERR_IGN_HTTP_HEADER, this does not invoke
5353
* nghttp2_on_invalid_header_callback.
5454
*/
55-
NGHTTP2_ERR_REMOVE_HTTP_HEADER = -106
55+
NGHTTP2_ERR_REMOVE_HTTP_HEADER = -106,
56+
/*
57+
* Cancel pushed stream.
58+
*/
59+
NGHTTP2_ERR_PUSH_CANCEL = -107,
5660
} nghttp2_internal_error;
5761

5862
#endif /* NGHTTP2_INT_H */

‎lib/nghttp2_session.c‎

Lines changed: 78 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -3263,7 +3263,9 @@ static int session_call_on_invalid_header(nghttp2_session *session,
32633263
session, frame, nv->name->base, nv->name->len, nv->value->base,
32643264
nv->value->len, nv->flags, session->user_data);
32653265
} else {
3266-
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
3266+
/* If both callbacks are not set, the invalid field nv is
3267+
ignored. */
3268+
return 0;
32673269
}
32683270

32693271
if (rv == NGHTTP2_ERR_PAUSE || rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
@@ -3348,6 +3350,10 @@ static uint32_t get_error_code_from_lib_error_code(int lib_error_code) {
33483350
case NGHTTP2_ERR_HTTP_HEADER:
33493351
case NGHTTP2_ERR_HTTP_MESSAGING:
33503352
return NGHTTP2_PROTOCOL_ERROR;
3353+
case NGHTTP2_ERR_INTERNAL:
3354+
return NGHTTP2_INTERNAL_ERROR;
3355+
case NGHTTP2_ERR_PUSH_CANCEL:
3356+
return NGHTTP2_CANCEL;
33513357
default:
33523358
return NGHTTP2_INTERNAL_ERROR;
33533359
}
@@ -3384,7 +3390,7 @@ static int session_handle_invalid_stream2(nghttp2_session *session,
33843390
if (rv != 0) {
33853391
return rv;
33863392
}
3387-
if (session->callbacks.on_invalid_frame_recv_callback) {
3393+
if (frame && session->callbacks.on_invalid_frame_recv_callback) {
33883394
if (session->callbacks.on_invalid_frame_recv_callback(
33893395
session, frame, lib_error_code, session->user_data) != 0) {
33903396
return NGHTTP2_ERR_CALLBACK_FAILURE;
@@ -3539,7 +3545,29 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
35393545

35403546
rv2 = session_call_on_invalid_header(session, frame, &nv);
35413547
if (rv2 == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
3542-
rv = NGHTTP2_ERR_HTTP_HEADER;
3548+
DEBUGF("recv: HTTP error: type=%u, id=%d, header %.*s: %.*s\n",
3549+
frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
3550+
nv.name->base, (int)nv.value->len, nv.value->base);
3551+
3552+
rv = session_call_error_callback(
3553+
session, NGHTTP2_ERR_HTTP_HEADER,
3554+
"Invalid HTTP header field was received: frame type: "
3555+
"%u, stream: %d, name: [%.*s], value: [%.*s]",
3556+
frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
3557+
nv.name->base, (int)nv.value->len, nv.value->base);
3558+
3559+
if (nghttp2_is_fatal(rv)) {
3560+
return rv;
3561+
}
3562+
3563+
rv = session_handle_invalid_stream2(
3564+
session, subject_stream->stream_id, frame,
3565+
NGHTTP2_ERR_HTTP_HEADER);
3566+
if (nghttp2_is_fatal(rv)) {
3567+
return rv;
3568+
}
3569+
3570+
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
35433571
} else {
35443572
if (rv2 != 0) {
35453573
return rv2;
@@ -3579,13 +3607,8 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
35793607
return rv;
35803608
}
35813609

3582-
rv =
3583-
session_handle_invalid_stream2(session, subject_stream->stream_id,
3584-
frame, NGHTTP2_ERR_HTTP_HEADER);
3585-
if (nghttp2_is_fatal(rv)) {
3586-
return rv;
3587-
}
3588-
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
3610+
return nghttp2_session_terminate_session(session,
3611+
NGHTTP2_PROTOCOL_ERROR);
35893612
}
35903613
}
35913614
if (rv == 0) {
@@ -3698,27 +3721,7 @@ static int session_after_header_block_received(nghttp2_session *session) {
36983721
}
36993722
}
37003723
if (rv != 0) {
3701-
int32_t stream_id;
3702-
3703-
if (frame->hd.type == NGHTTP2_PUSH_PROMISE) {
3704-
stream_id = frame->push_promise.promised_stream_id;
3705-
} else {
3706-
stream_id = frame->hd.stream_id;
3707-
}
3708-
3709-
rv = session_handle_invalid_stream2(session, stream_id, frame,
3710-
NGHTTP2_ERR_HTTP_MESSAGING);
3711-
if (nghttp2_is_fatal(rv)) {
3712-
return rv;
3713-
}
3714-
3715-
if (frame->hd.type == NGHTTP2_HEADERS &&
3716-
(frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) {
3717-
nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
3718-
/* Don't call nghttp2_session_close_stream_if_shut_rdwr
3719-
because RST_STREAM has been submitted. */
3720-
}
3721-
return 0;
3724+
return nghttp2_session_terminate_session(session, NGHTTP2_PROTOCOL_ERROR);
37223725
}
37233726
}
37243727

@@ -4054,8 +4057,7 @@ static int update_remote_initial_window_size_func(void *entry, void *ptr) {
40544057
rv = nghttp2_stream_update_remote_initial_window_size(
40554058
stream, arg->new_window_size, arg->old_window_size);
40564059
if (rv != 0) {
4057-
return nghttp2_session_add_rst_stream(arg->session, stream->stream_id,
4058-
NGHTTP2_FLOW_CONTROL_ERROR);
4060+
return NGHTTP2_ERR_FLOW_CONTROL;
40594061
}
40604062

40614063
/* If window size gets positive, push deferred DATA frame to
@@ -4081,6 +4083,8 @@ static int update_remote_initial_window_size_func(void *entry, void *ptr) {
40814083
*
40824084
* NGHTTP2_ERR_NOMEM
40834085
* Out of memory.
4086+
* NGHTTP2_ERR_FLOW_CONTROL
4087+
* Window size gets out of range.
40844088
*/
40854089
static int
40864090
session_update_remote_initial_window_size(nghttp2_session *session,
@@ -4104,8 +4108,7 @@ static int update_local_initial_window_size_func(void *entry, void *ptr) {
41044108
rv = nghttp2_stream_update_local_initial_window_size(
41054109
stream, arg->new_window_size, arg->old_window_size);
41064110
if (rv != 0) {
4107-
return nghttp2_session_add_rst_stream(arg->session, stream->stream_id,
4108-
NGHTTP2_FLOW_CONTROL_ERROR);
4111+
return NGHTTP2_ERR_FLOW_CONTROL;
41094112
}
41104113

41114114
if (stream->window_update_queued) {
@@ -4139,6 +4142,8 @@ static int update_local_initial_window_size_func(void *entry, void *ptr) {
41394142
*
41404143
* NGHTTP2_ERR_NOMEM
41414144
* Out of memory.
4145+
* NGHTTP2_ERR_FLOW_CONTROL
4146+
* Window size gets out of range.
41424147
*/
41434148
static int
41444149
session_update_local_initial_window_size(nghttp2_session *session,
@@ -4525,9 +4530,9 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
45254530
session->max_incoming_reserved_streams) {
45264531
/* Currently, client does not retain closed stream, so we don't
45274532
check NGHTTP2_SHUT_RD condition here. */
4528-
4529-
rv = nghttp2_session_add_rst_stream(
4530-
session, frame->push_promise.promised_stream_id, NGHTTP2_CANCEL);
4533+
rv = session_handle_invalid_stream2(session,
4534+
frame->push_promise.promised_stream_id,
4535+
NULL, NGHTTP2_ERR_PUSH_CANCEL);
45314536
if (rv != 0) {
45324537
return rv;
45334538
}
@@ -4684,8 +4689,9 @@ static int session_on_stream_window_update_received(nghttp2_session *session,
46844689
}
46854690
if (NGHTTP2_MAX_WINDOW_SIZE - frame->window_update.window_size_increment <
46864691
stream->remote_window_size) {
4687-
return session_handle_invalid_stream(session, frame,
4688-
NGHTTP2_ERR_FLOW_CONTROL);
4692+
return session_handle_invalid_connection(
4693+
session, frame, NGHTTP2_ERR_FLOW_CONTROL,
4694+
"WINDOW_UPDATE: window size overflow");
46894695
}
46904696
stream->remote_window_size += frame->window_update.window_size_increment;
46914697

@@ -4915,16 +4921,7 @@ int nghttp2_session_on_data_received(nghttp2_session *session,
49154921
if (session_enforce_http_messaging(session) &&
49164922
(frame->hd.flags & NGHTTP2_FLAG_END_STREAM)) {
49174923
if (nghttp2_http_on_remote_end_stream(stream) != 0) {
4918-
rv = nghttp2_session_add_rst_stream(session, stream->stream_id,
4919-
NGHTTP2_PROTOCOL_ERROR);
4920-
if (nghttp2_is_fatal(rv)) {
4921-
return rv;
4922-
}
4923-
4924-
nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD);
4925-
/* Don't call nghttp2_session_close_stream_if_shut_rdwr because
4926-
RST_STREAM has been submitted. */
4927-
return 0;
4924+
return nghttp2_session_terminate_session(session, NGHTTP2_PROTOCOL_ERROR);
49284925
}
49294926
}
49304927

@@ -4982,8 +4979,8 @@ int nghttp2_session_update_recv_stream_window_size(nghttp2_session *session,
49824979
rv = adjust_recv_window_size(&stream->recv_window_size, delta_size,
49834980
stream->local_window_size);
49844981
if (rv != 0) {
4985-
return nghttp2_session_add_rst_stream(session, stream->stream_id,
4986-
NGHTTP2_FLOW_CONTROL_ERROR);
4982+
return nghttp2_session_terminate_session(session,
4983+
NGHTTP2_FLOW_CONTROL_ERROR);
49874984
}
49884985
/* We don't have to send WINDOW_UPDATE if the data received is the
49894986
last chunk in the incoming stream. */
@@ -5542,8 +5539,8 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
55425539
}
55435540

55445541
if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
5545-
rv = nghttp2_session_add_rst_stream(
5546-
session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR);
5542+
rv = session_handle_invalid_stream2(
5543+
session, iframe->frame.hd.stream_id, NULL, NGHTTP2_ERR_INTERNAL);
55475544
if (nghttp2_is_fatal(rv)) {
55485545
return rv;
55495546
}
@@ -5956,8 +5953,8 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
59565953
}
59575954

59585955
if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
5959-
rv = nghttp2_session_add_rst_stream(
5960-
session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR);
5956+
rv = session_handle_invalid_stream2(
5957+
session, iframe->frame.hd.stream_id, NULL, NGHTTP2_ERR_INTERNAL);
59615958
if (nghttp2_is_fatal(rv)) {
59625959
return rv;
59635960
}
@@ -6031,9 +6028,9 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
60316028
}
60326029

60336030
if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
6034-
rv = nghttp2_session_add_rst_stream(
6035-
session, iframe->frame.push_promise.promised_stream_id,
6036-
NGHTTP2_INTERNAL_ERROR);
6031+
rv = session_handle_invalid_stream2(
6032+
session, iframe->frame.push_promise.promised_stream_id, NULL,
6033+
NGHTTP2_ERR_INTERNAL);
60376034
if (nghttp2_is_fatal(rv)) {
60386035
return rv;
60396036
}
@@ -6211,12 +6208,12 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
62116208
iframe->payloadleft -= hd_proclen;
62126209

62136210
/* Use promised stream ID for PUSH_PROMISE */
6214-
rv = nghttp2_session_add_rst_stream(
6211+
rv = session_handle_invalid_stream2(
62156212
session,
62166213
iframe->frame.hd.type == NGHTTP2_PUSH_PROMISE
62176214
? iframe->frame.push_promise.promised_stream_id
62186215
: iframe->frame.hd.stream_id,
6219-
NGHTTP2_INTERNAL_ERROR);
6216+
NULL, NGHTTP2_ERR_INTERNAL);
62206217
if (nghttp2_is_fatal(rv)) {
62216218
return rv;
62226219
}
@@ -6263,6 +6260,10 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
62636260
if (nghttp2_is_fatal(rv)) {
62646261
return rv;
62656262
}
6263+
6264+
if (iframe->state == NGHTTP2_IB_IGN_ALL) {
6265+
return (nghttp2_ssize)inlen;
6266+
}
62666267
}
62676268
session_inbound_frame_reset(session);
62686269

@@ -6488,6 +6489,10 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
64886489
if (nghttp2_is_fatal(rv)) {
64896490
return rv;
64906491
}
6492+
6493+
if (iframe->state == NGHTTP2_IB_IGN_ALL) {
6494+
return (nghttp2_ssize)inlen;
6495+
}
64916496
}
64926497

64936498
busy = 1;
@@ -6546,6 +6551,10 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
65466551
return rv;
65476552
}
65486553

6554+
if (iframe->state == NGHTTP2_IB_IGN_ALL) {
6555+
return (nghttp2_ssize)inlen;
6556+
}
6557+
65496558
data_readlen =
65506559
inbound_frame_effective_readlen(iframe, iframe->payloadleft, readlen);
65516560

@@ -6575,28 +6584,13 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
65756584
if (data_readlen > 0) {
65766585
if (session_enforce_http_messaging(session)) {
65776586
if (nghttp2_http_on_data_chunk(stream, (size_t)data_readlen) != 0) {
6578-
if (session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE) {
6579-
/* Consume all data for connection immediately here */
6580-
rv = session_update_connection_consumed_size(
6581-
session, (size_t)data_readlen);
6582-
6583-
if (nghttp2_is_fatal(rv)) {
6584-
return rv;
6585-
}
6586-
6587-
if (iframe->state == NGHTTP2_IB_IGN_DATA) {
6588-
return (nghttp2_ssize)inlen;
6589-
}
6590-
}
6591-
6592-
rv = nghttp2_session_add_rst_stream(
6593-
session, iframe->frame.hd.stream_id, NGHTTP2_PROTOCOL_ERROR);
6587+
rv = nghttp2_session_terminate_session(session,
6588+
NGHTTP2_PROTOCOL_ERROR);
65946589
if (nghttp2_is_fatal(rv)) {
65956590
return rv;
65966591
}
6597-
busy = 1;
6598-
iframe->state = NGHTTP2_IB_IGN_DATA;
6599-
break;
6592+
6593+
return (nghttp2_ssize)inlen;
66006594
}
66016595
}
66026596
if (session->callbacks.on_data_chunk_recv_callback) {
@@ -6623,6 +6617,10 @@ nghttp2_ssize nghttp2_session_mem_recv2(nghttp2_session *session,
66236617
return rv;
66246618
}
66256619

6620+
if (iframe->state == NGHTTP2_IB_IGN_ALL) {
6621+
return (nghttp2_ssize)inlen;
6622+
}
6623+
66266624
session_inbound_frame_reset(session);
66276625

66286626
break;

0 commit comments

Comments
 (0)