diff --git a/src/proxy/http2/Http2ConnectionState.cc b/src/proxy/http2/Http2ConnectionState.cc index 4b38579574c..fc5a94e5c8f 100644 --- a/src/proxy/http2/Http2ConnectionState.cc +++ b/src/proxy/http2/Http2ConnectionState.cc @@ -345,14 +345,23 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) stream = this->create_stream(stream_id, error); new_stream = true; if (!stream) { - // Terminate the connection with COMPRESSION_ERROR because we don't decompress the field block in this HEADERS frame. - // TODO: try to decompress to keep HPACK Dynamic Table in sync. + // Per RFC 9113, we must decode the header block even when refusing the stream to keep + // the HPACK dynamic table in sync. Create a temporary unregistered stream for decoding. if (error.cls == Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM) { - return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_COMPRESSION_ERROR, - error.msg); + uint32_t const initial_local_stream_window = this->acknowledged_local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE); + stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread(), this->session->get_proxy_session(), stream_id, + this->peer_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE), initial_local_stream_window, + !STREAM_IS_REGISTERED); + if (!stream) { + // Failed to create even a temporary stream, this is a serious error. + return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR, + "failed to create temporary stream for header decoding"); + } + free_stream_after_decoding = true; + reset_header_after_decoding = true; + } else { + return error; } - - return error; } } } @@ -483,6 +492,9 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) if (reset_header_after_decoding) { stream->reset_receive_headers(); if (free_stream_after_decoding) { + // Send RST_STREAM to inform the client that the stream was refused. + // This typically happens when max_concurrent_streams is exceeded. + this->send_rst_stream_frame(stream_id, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM); THREAD_FREE(stream, http2StreamAllocator, this_ethread()); } return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); @@ -692,6 +704,11 @@ Http2ConnectionState::rcv_rst_stream_frame(const Http2Frame &frame) Http2StreamDebug(this->session, stream_id, "Parsed RST_STREAM frame: Error Code: %u", rst_stream.error_code); stream->set_rx_error_code({ProxyErrorClass::TXN, static_cast(rst_stream.error_code)}); stream->initiating_close(); + // Delete the stream immediately to free up the stream count before the next frame is processed. + // This prevents the race condition where HEADERS frames are incorrectly refused due to the + // stream count not being decremented yet. The destructor will handle the case where the stream + // has already been removed from the stream list. + this->delete_stream(stream); } return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); diff --git a/tests/gold_tests/h2/http2_stream_cancel_timing.test.py b/tests/gold_tests/h2/http2_stream_cancel_timing.test.py new file mode 100644 index 00000000000..28541f843ea --- /dev/null +++ b/tests/gold_tests/h2/http2_stream_cancel_timing.test.py @@ -0,0 +1,67 @@ +''' +Test that canceled HTTP/2 streams are cleaned up in time. + +This test reproduces issue #9179 where canceled streams via RST_STREAM +are not cleaned up in time, causing subsequent HEADERS frames to be +incorrectly refused with REFUSED_STREAM error. +''' + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Test that canceled HTTP/2 streams are cleaned up in time. +''' + +Test.SkipUnless(Condition.HasProxyVerifierVersion('2.8.0')) + +# +# Test stream cancellation timing +# +ts = Test.MakeATSProcess("ts", enable_tls=True) +replay_file = "replay/http2_stream_cancel_timing.replay.yaml" +server = Test.MakeVerifierServerProcess("server", replay_file) +ts.addDefaultSSLFiles() +ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}', + 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}', + 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE', + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http2', + 'proxy.config.exec_thread.autoconfig.enabled': 0, + 'proxy.config.exec_thread.limit': 1, + # Set max_concurrent_streams to 5 to reproduce the issue + 'proxy.config.http2.max_concurrent_streams_in': 5, + }) +ts.Disk.remap_config.AddLine(f'map / https://127.0.0.1:{server.Variables.https_port}') +ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key') + +tr = Test.AddTestRun('Test that canceled streams are cleaned up in time') +tr.Processes.Default.StartBefore(server) +tr.Processes.Default.StartBefore(ts) +tr.AddVerifierClientProcess("client", replay_file, http_ports=[ts.Variables.port], https_ports=[ts.Variables.ssl_port]) + +# The test should pass - all 5 streams in the second batch should be accepted +tr.StillRunningAfter = ts +tr.StillRunningAfter = server + +# Check that streams 11, 13, 15, 17, 19 are NOT refused +ts.Disk.traffic_out.Content += Testers.ExcludesExpression('stream=11.*REFUSED_STREAM', 'Stream 11 should not be refused') +ts.Disk.traffic_out.Content += Testers.ExcludesExpression('stream=13.*REFUSED_STREAM', 'Stream 13 should not be refused') +ts.Disk.traffic_out.Content += Testers.ExcludesExpression('stream=15.*REFUSED_STREAM', 'Stream 15 should not be refused') +ts.Disk.traffic_out.Content += Testers.ExcludesExpression('stream=17.*REFUSED_STREAM', 'Stream 17 should not be refused') +ts.Disk.traffic_out.Content += Testers.ExcludesExpression('stream=19.*REFUSED_STREAM', 'Stream 19 should not be refused') diff --git a/tests/gold_tests/h2/replay/http2_stream_cancel_timing.replay.yaml b/tests/gold_tests/h2/replay/http2_stream_cancel_timing.replay.yaml new file mode 100644 index 00000000000..6c28eac9906 --- /dev/null +++ b/tests/gold_tests/h2/replay/http2_stream_cancel_timing.replay.yaml @@ -0,0 +1,253 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# This replay file tests that canceled streams are cleaned up in time +# to allow new streams to be created without hitting max_concurrent_streams limit. +# + +meta: + version: "1.0" + +sessions: +- protocol: + - name: http + version: 2 + - name: tls + sni: test_sni + - name: tcp + - name: ip + transactions: + # Stream 1 - will be canceled + - client-request: + frames: + - HEADERS: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/1 ] + - [ uuid, 1 ] + - RST_STREAM: + error-code: CANCEL + + server-response: + delay: 5s + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + # Stream 3 - will be canceled + - client-request: + frames: + - HEADERS: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/3 ] + - [ uuid, 3 ] + - RST_STREAM: + error-code: CANCEL + + server-response: + delay: 5s + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + # Stream 5 - will be canceled + - client-request: + frames: + - HEADERS: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/5 ] + - [ uuid, 5 ] + - RST_STREAM: + error-code: CANCEL + + server-response: + delay: 5s + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + # Stream 7 - will be canceled + - client-request: + frames: + - HEADERS: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/7 ] + - [ uuid, 7 ] + - RST_STREAM: + error-code: CANCEL + + server-response: + delay: 5s + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + # Stream 9 - will be canceled + - client-request: + frames: + - HEADERS: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/9 ] + - [ uuid, 9 ] + - RST_STREAM: + error-code: CANCEL + + server-response: + delay: 5s + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + # Stream 11 - should be accepted, not refused + - client-request: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/11 ] + - [ uuid, 11 ] + + server-response: + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + proxy-response: + status: 200 + + # Stream 13 - should be accepted, not refused + - client-request: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/13 ] + - [ uuid, 13 ] + + server-response: + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + proxy-response: + status: 200 + + # Stream 15 - should be accepted, not refused + - client-request: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/15 ] + - [ uuid, 15 ] + + server-response: + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + proxy-response: + status: 200 + + # Stream 17 - should be accepted, not refused + - client-request: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/17 ] + - [ uuid, 17 ] + + server-response: + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + proxy-response: + status: 200 + + # Stream 19 - should be accepted, not refused + - client-request: + headers: + fields: + - [ :method, GET ] + - [ :scheme, https ] + - [ :authority, www.example.com ] + - [ :path, /path/19 ] + - [ uuid, 19 ] + + server-response: + headers: + fields: + - [ :status, 200 ] + - [ Content-Length, '100' ] + content: + size: 100 + + proxy-response: + status: 200 +