From: Stefan Eissing Date: Tue, 11 Apr 2023 08:07:59 +0000 (+0200) Subject: http2: fix copynpaste error reported by coverity X-Git-Tag: curl-8_1_0~188 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c59b5b3c871e02a67cb490b7578f4e93096c3aa4;p=thirdparty%2Fcurl.git http2: fix copynpaste error reported by coverity - move all code handling HTTP/2 frames for a particular stream into a separate function to keep from confusing the call `data` with the stream `data`. Closes #10924 --- diff --git a/lib/http2.c b/lib/http2.c index 127ae4394f..86b3534853 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -950,98 +950,44 @@ static CURLcode recvbuf_write_hds(struct Curl_cfilter *cf, return CURLE_OK; } -static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, - void *userp) +static CURLcode on_stream_frame(struct Curl_cfilter *cf, + struct Curl_easy *data, + const nghttp2_frame *frame) { - struct Curl_cfilter *cf = userp; struct cf_h2_ctx *ctx = cf->ctx; - struct Curl_easy *data_s = NULL; - struct stream_ctx *stream = NULL; - struct Curl_easy *data = CF_DATA_CURRENT(cf); - int rv; + struct stream_ctx *stream = H2_STREAM_CTX(data); int32_t stream_id = frame->hd.stream_id; CURLcode result; + int rv; - DEBUGASSERT(data); - if(!stream_id) { - /* stream ID zero is for connection-oriented stuff */ - DEBUGASSERT(data); - switch(frame->hd.type) { - case NGHTTP2_SETTINGS: { - uint32_t max_conn = ctx->max_concurrent_streams; - DEBUGF(LOG_CF(data, cf, "recv frame SETTINGS")); - ctx->max_concurrent_streams = nghttp2_session_get_remote_settings( - session, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); - ctx->enable_push = nghttp2_session_get_remote_settings( - session, NGHTTP2_SETTINGS_ENABLE_PUSH) != 0; - DEBUGF(LOG_CF(data, cf, "MAX_CONCURRENT_STREAMS == %d", - ctx->max_concurrent_streams)); - DEBUGF(LOG_CF(data, cf, "ENABLE_PUSH == %s", - ctx->enable_push ? "TRUE" : "false")); - if(data && max_conn != ctx->max_concurrent_streams) { - /* only signal change if the value actually changed */ - DEBUGF(LOG_CF(data, cf, "MAX_CONCURRENT_STREAMS now %u", - ctx->max_concurrent_streams)); - multi_connchanged(data->multi); - } - break; - } - case NGHTTP2_GOAWAY: - ctx->goaway = TRUE; - ctx->goaway_error = frame->goaway.error_code; - ctx->last_stream_id = frame->goaway.last_stream_id; - if(data) { - DEBUGF(LOG_CF(data, cf, "recv GOAWAY, error=%d, last_stream=%u", - ctx->goaway_error, ctx->last_stream_id)); - infof(data, "recveived GOAWAY, error=%d, last_stream=%u", - ctx->goaway_error, ctx->last_stream_id); - multi_connchanged(data->multi); - } - break; - case NGHTTP2_WINDOW_UPDATE: - DEBUGF(LOG_CF(data, cf, "recv frame WINDOW_UPDATE")); - break; - default: - DEBUGF(LOG_CF(data, cf, "recv frame %x on 0", frame->hd.type)); - } - return 0; - } - data_s = nghttp2_session_get_stream_user_data(session, stream_id); - if(!data_s) { - DEBUGF(LOG_CF(data, cf, "[h2sid=%d] No Curl_easy associated", - stream_id)); - return 0; - } - - stream = H2_STREAM_CTX(data_s); if(!stream) { - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] No proto pointer", stream_id)); - return NGHTTP2_ERR_CALLBACK_FAILURE; + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] No proto pointer", stream_id)); + return CURLE_FAILED_INIT; } switch(frame->hd.type) { case NGHTTP2_DATA: /* If !body started on this stream, then receiving DATA is illegal. */ - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] recv frame DATA", stream_id)); + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv frame DATA", stream_id)); if(!stream->bodystarted) { - rv = nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, + rv = nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_PROTOCOL_ERROR); if(nghttp2_is_fatal(rv)) { - return NGHTTP2_ERR_CALLBACK_FAILURE; + return CURLE_RECV_ERROR; } } if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { /* Stream has ended. If there is pending data, ensure that read will occur to consume it. */ if(!data->state.drain && !Curl_bufq_is_empty(&stream->recvbuf)) { - drain_this(cf, data_s); + drain_this(cf, data); Curl_expire(data, 0, EXPIRE_RUN_NOW); } } break; case NGHTTP2_HEADERS: - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] recv frame HEADERS", stream_id)); + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv frame HEADERS", stream_id)); if(stream->bodystarted) { /* Only valid HEADERS after body started is trailer HEADERS. We buffer them in on_header callback. */ @@ -1052,7 +998,7 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, stream->status_code. Fuzzing has proven this can still be reached without status code having been set. */ if(stream->status_code == -1) - return NGHTTP2_ERR_CALLBACK_FAILURE; + return CURLE_RECV_ERROR; /* Only final status code signals the end of header */ if(stream->status_code / 100 != 1) { @@ -1060,36 +1006,36 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, stream->status_code = -1; } - result = recvbuf_write_hds(cf, data_s, STRCONST("\r\n")); + result = recvbuf_write_hds(cf, data, STRCONST("\r\n")); if(result) - return NGHTTP2_ERR_CALLBACK_FAILURE; + return result; - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] %zu header bytes", + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] %zu header bytes", stream_id, Curl_bufq_len(&stream->recvbuf))); - if(CF_DATA_CURRENT(cf) != data_s) { - drain_this(cf, data_s); - Curl_expire(data_s, 0, EXPIRE_RUN_NOW); + if(CF_DATA_CURRENT(cf) != data) { + drain_this(cf, data); + Curl_expire(data, 0, EXPIRE_RUN_NOW); } break; case NGHTTP2_PUSH_PROMISE: - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] recv PUSH_PROMISE", stream_id)); - rv = push_promise(cf, data_s, &frame->push_promise); + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv PUSH_PROMISE", stream_id)); + rv = push_promise(cf, data, &frame->push_promise); if(rv) { /* deny! */ - int h2; DEBUGASSERT((rv > CURL_PUSH_OK) && (rv <= CURL_PUSH_ERROROUT)); - h2 = nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, + rv = nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, frame->push_promise.promised_stream_id, NGHTTP2_CANCEL); - if(nghttp2_is_fatal(h2)) - return NGHTTP2_ERR_CALLBACK_FAILURE; + if(nghttp2_is_fatal(rv)) + return CURLE_SEND_ERROR; else if(rv == CURL_PUSH_ERROROUT) { - DEBUGF(LOG_CF(data_s, cf, "Fail the parent stream (too)")); - return NGHTTP2_ERR_CALLBACK_FAILURE; + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] fail in PUSH_PROMISE received", + stream_id)); + return CURLE_RECV_ERROR; } } break; case NGHTTP2_RST_STREAM: - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] recv RST", stream_id)); + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv RST", stream_id)); stream->closed = TRUE; stream->reset = TRUE; drain_this(cf, data); @@ -1097,21 +1043,84 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, break; case NGHTTP2_WINDOW_UPDATE: DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv WINDOW_UPDATE", stream_id)); - if((data_s->req.keepon & KEEP_SEND_HOLD) && - (data_s->req.keepon & KEEP_SEND)) { - data_s->req.keepon &= ~KEEP_SEND_HOLD; - drain_this(cf, data_s); - Curl_expire(data_s, 0, EXPIRE_RUN_NOW); + if((data->req.keepon & KEEP_SEND_HOLD) && + (data->req.keepon & KEEP_SEND)) { + data->req.keepon &= ~KEEP_SEND_HOLD; + drain_this(cf, data); + Curl_expire(data, 0, EXPIRE_RUN_NOW); DEBUGF(LOG_CF(data, cf, "[h2sid=%d] un-holding after win update", stream_id)); } break; default: - DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] recv frame %x", + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] recv frame %x", stream_id, frame->hd.type)); break; } - return 0; + return CURLE_OK; +} + +static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame, + void *userp) +{ + struct Curl_cfilter *cf = userp; + struct cf_h2_ctx *ctx = cf->ctx; + struct Curl_easy *data = CF_DATA_CURRENT(cf), *data_s; + int32_t stream_id = frame->hd.stream_id; + + DEBUGASSERT(data); + if(!stream_id) { + /* stream ID zero is for connection-oriented stuff */ + DEBUGASSERT(data); + switch(frame->hd.type) { + case NGHTTP2_SETTINGS: { + uint32_t max_conn = ctx->max_concurrent_streams; + DEBUGF(LOG_CF(data, cf, "recv frame SETTINGS")); + ctx->max_concurrent_streams = nghttp2_session_get_remote_settings( + session, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); + ctx->enable_push = nghttp2_session_get_remote_settings( + session, NGHTTP2_SETTINGS_ENABLE_PUSH) != 0; + DEBUGF(LOG_CF(data, cf, "MAX_CONCURRENT_STREAMS == %d", + ctx->max_concurrent_streams)); + DEBUGF(LOG_CF(data, cf, "ENABLE_PUSH == %s", + ctx->enable_push ? "TRUE" : "false")); + if(data && max_conn != ctx->max_concurrent_streams) { + /* only signal change if the value actually changed */ + DEBUGF(LOG_CF(data, cf, "MAX_CONCURRENT_STREAMS now %u", + ctx->max_concurrent_streams)); + multi_connchanged(data->multi); + } + break; + } + case NGHTTP2_GOAWAY: + ctx->goaway = TRUE; + ctx->goaway_error = frame->goaway.error_code; + ctx->last_stream_id = frame->goaway.last_stream_id; + if(data) { + DEBUGF(LOG_CF(data, cf, "recv GOAWAY, error=%d, last_stream=%u", + ctx->goaway_error, ctx->last_stream_id)); + infof(data, "recveived GOAWAY, error=%d, last_stream=%u", + ctx->goaway_error, ctx->last_stream_id); + multi_connchanged(data->multi); + } + break; + case NGHTTP2_WINDOW_UPDATE: + DEBUGF(LOG_CF(data, cf, "recv frame WINDOW_UPDATE")); + break; + default: + DEBUGF(LOG_CF(data, cf, "recv frame %x on 0", frame->hd.type)); + } + return 0; + } + + data_s = nghttp2_session_get_stream_user_data(session, stream_id); + if(!data_s) { + DEBUGF(LOG_CF(data, cf, "[h2sid=%d] No Curl_easy associated", + stream_id)); + return 0; + } + + return on_stream_frame(cf, data_s, frame)? NGHTTP2_ERR_CALLBACK_FAILURE : 0; } static int on_data_chunk_recv(nghttp2_session *session, uint8_t flags,