Fix a crash caused by a HTTP/2 GET request with a body

proxy/http2/Http2ConnectionState.cc

@@ -30,6 +30,7 @@
 #include "Http2Frame.h"
 #include "Http2DebugNames.h"
 #include "HttpDebugNames.h"
+#include "HttpSM.h"
 
 #include "tscore/ink_assert.h"
 #include "tscpp/util/PostScript.h"
@@ -633,9 +634,7 @@ Http2ConnectionState::rcv_rst_stream_frame(const Http2Frame &frame)
 
   if (stream != nullptr) {
     Http2StreamDebug(this->session, stream_id, "Parsed RST_STREAM: Error Code: %u", rst_stream.error_code);
-
     stream->set_rx_error_code({ProxyErrorClass::TXN, static_cast<uint32_t>(rst_stream.error_code)});
-    stream->signal_read_event(VC_EVENT_EOS);
     stream->initiating_close();
   }
 
@@ -2197,16 +2196,21 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
       // Set END_STREAM on request headers for POST, etc. methods combined with
       // an explicit length 0. Some origins RST on request headers with
       // explicit zero length and no end stream flag, causing the request to
-      // fail. We emulate chromium behaviour here prevent such RSTs.
+      // fail. We emulate chromium behaviour here prevent such RSTs. Transfer-encoding
+      // implies there’s a body, regardless of whether it is chunked or not.
       bool content_method       = method == HTTP_WKSIDX_POST || method == HTTP_WKSIDX_PUSH || method == HTTP_WKSIDX_PUT;
       bool is_transfer_encoded  = send_hdr->presence(MIME_PRESENCE_TRANSFER_ENCODING);
       bool has_content_header   = send_hdr->presence(MIME_PRESENCE_CONTENT_LENGTH);
       bool explicit_zero_length = has_content_header && send_hdr->get_content_length() == 0;
+      int64_t content_length    = has_content_header ? send_hdr->get_content_length() : 0L;
+      bool is_chunked =
+        is_transfer_encoded && send_hdr->value_get(MIME_FIELD_TRANSFER_ENCODING) == std::string_view(HTTP_VALUE_CHUNKED);
 
       bool expect_content_stream =
-        is_transfer_encoded ||                                              // transfer encoded content length is unknown
-        (!content_method && has_content_header && !explicit_zero_length) || // non zero content with GET,etc
-        (content_method && !explicit_zero_length);                          // content-length >0 or empty with POST etc
+        is_transfer_encoded ||                                                        // transfer encoded content length is unknown
+        (!content_method && has_content_header && !explicit_zero_length) ||           // nonzero content with GET,etc
+        (content_method && !explicit_zero_length) ||                                  // content-length >0 or empty with POST etc
+        stream->get_sm()->get_ua_txn()->has_request_body(content_length, is_chunked); // request has a body
 
       // send END_STREAM if we don't expect any content
       if (!expect_content_stream) {

proxy/http2/Http2Stream.cc

@@ -495,7 +495,11 @@ Http2Stream::do_io_close(int /* flags */)
     REMEMBER(NO_EVENT, this->reentrancy_count);
     Http2StreamDebug("do_io_close");
 
-    if (this->is_state_writeable()) { // Let the other end know we are going away
+    // Let the other end know we are going away.
+    // We only need to do this for the client side since we only need to pass through RST_STREAM
+    // from the server. If a client sends a RST_STREAM, we need to keep the server side alive so
+    // the background fill can function as intended.
+    if (!this->is_outbound_connection() && this->is_state_writeable()) {
       this->get_connection_state().send_rst_stream_frame(_id, Http2ErrorCode::HTTP2_ERROR_NO_ERROR);
     }
 
@@ -561,7 +565,7 @@ Http2Stream::initiating_close()
     Http2StreamDebug("initiating_close client_window=%zd session_window=%zd", _peer_rwnd,
                      this->get_connection_state().get_peer_rwnd());
 
-    if (this->is_state_writeable()) { // Let the other end know we are going away
+    if (!this->is_outbound_connection() && this->is_state_writeable()) { // Let the other end know we are going away
       this->get_connection_state().send_rst_stream_frame(_id, Http2ErrorCode::HTTP2_ERROR_NO_ERROR);
     }
 

tests/gold_tests/cache/background_fill.test.py

@@ -17,10 +17,12 @@
 #  limitations under the License.
 
 from enum import Enum
+import os
 
 Test.Summary = 'Exercise Background Fill'
 Test.SkipUnless(
     Condition.HasCurlFeature('http2'),
+    Condition.HasProxyVerifierVersion('2.8.0')
 )
 Test.ContinueOnFail = True
 
@@ -39,48 +41,68 @@ class State(Enum):
 
     def __init__(self):
         self.state = self.State.INIT
+        self.ts = {}
         self.__setupOriginServer()
-        self.__setupTS()
+        self.__setupTS(['for_httpbin', 'for_pv'])
 
     def __setupOriginServer(self):
         self.httpbin = Test.MakeHttpBinServer("httpbin")
-
-    def __setupTS(self):
-        self.ts = Test.MakeATSProcess("ts", enable_tls=True)
-
-        self.ts.addDefaultSSLFiles()
-        self.ts.Disk.ssl_multicert_config.AddLine(
-            "dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key")
-
-        self.ts.Disk.records_config.update({
-            "proxy.config.http.server_ports": f"{self.ts.Variables.port} {self.ts.Variables.ssl_port}:ssl",
-            'proxy.config.ssl.server.cert.path': f"{self.ts.Variables.SSLDir}",
-            'proxy.config.ssl.server.private_key.path': f"{self.ts.Variables.SSLDir}",
-            "proxy.config.diags.debug.enabled": 1,
-            "proxy.config.diags.debug.tags": "http",
-            "proxy.config.http.background_fill_active_timeout": "0",
-            "proxy.config.http.background_fill_completed_threshold": "0.0",
-            "proxy.config.http.cache.required_headers": 0,  # Force cache
-            "proxy.config.http.insert_response_via_str": 2,
-        })
-
-        self.ts.Disk.remap_config.AddLines([
-            f"map / http://127.0.0.1:{self.httpbin.Variables.Port}/",
-        ])
+        self.pv_server = Test.MakeVerifierServerProcess("server0", "replay/bg_fill.yaml")
+
+    def __setupTS(self, ts_names=['default']):
+        for name in ts_names:
+            self.ts[name] = Test.MakeATSProcess(name, select_ports=True, enable_tls=True, enable_cache=True)
+
+            self.ts[name].addDefaultSSLFiles()
+            self.ts[name].Disk.ssl_multicert_config.AddLine(
+                "dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key")
+
+            self.ts[name].Disk.records_config.update({
+                "proxy.config.http.server_ports": f"{self.ts[name].Variables.port} {self.ts[name].Variables.ssl_port}:ssl",
+                "proxy.config.http.background_fill_active_timeout": "0",
+                "proxy.config.http.background_fill_completed_threshold": "0.0",
+                "proxy.config.http.cache.required_headers": 0,  # Force cache
+                "proxy.config.http.insert_response_via_str": 2,
+                'proxy.config.http.server_session_sharing.pool': 'thread',
+                'proxy.config.http.server_session_sharing.match': 'ip,sni,cert',
+                'proxy.config.exec_thread.autoconfig.enabled': 0,
+                'proxy.config.exec_thread.limit': 1,
+                'proxy.config.ssl.server.cert.path': f"{self.ts[name].Variables.SSLDir}",
+                'proxy.config.ssl.server.private_key.path': f"{self.ts[name].Variables.SSLDir}",
+                'proxy.config.ssl.client.alpn_protocols': 'h2,http/1.1',
+                'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
+                "proxy.config.diags.debug.enabled": 3,
+                "proxy.config.diags.debug.tags": "http",
+            })
+
+            if name == 'for_httpbin' or name == 'default':
+                self.ts[name].Disk.remap_config.AddLines([
+                    f"map / http://127.0.0.1:{self.httpbin.Variables.Port}",
+                ])
+            else:
+                self.ts[name].Disk.remap_config.AddLines([
+                    f'map / https://127.0.0.1:{self.pv_server.Variables.https_port}',
+                ])
 
     def __checkProcessBefore(self, tr):
         if self.state == self.State.RUNNING:
             tr.StillRunningBefore = self.httpbin
-            tr.StillRunningBefore = self.ts
+            tr.StillRunningBefore = self.pv_server
+            tr.StillRunningBefore = self.ts['for_httpbin']
+            tr.StillRunningBefore = self.ts['for_pv']
         else:
             tr.Processes.Default.StartBefore(self.httpbin, ready=When.PortOpen(self.httpbin.Variables.Port))
-            tr.Processes.Default.StartBefore(self.ts)
+            tr.Processes.Default.StartBefore(self.pv_server)
+            tr.Processes.Default.StartBefore(self.ts['for_httpbin'])
+            tr.Processes.Default.StartBefore(self.ts['for_pv'])
             self.state = self.State.RUNNING
 
     def __checkProcessAfter(self, tr):
         assert (self.state == self.State.RUNNING)
         tr.StillRunningAfter = self.httpbin
-        tr.StillRunningAfter = self.ts
+        tr.StillRunningAfter = self.pv_server
+        tr.StillRunningAfter = self.ts['for_httpbin']
+        tr.StillRunningAfter = self.ts['for_pv']
 
     def __testCase0(self):
         """
@@ -89,10 +111,10 @@ def __testCase0(self):
         tr = Test.AddTestRun()
         self.__checkProcessBefore(tr)
         tr.Processes.Default.Command = f"""
-curl -X PURGE --http1.1 -vs http://127.0.0.1:{self.ts.Variables.port}/drip?duration=4;
-timeout 2 curl --http1.1 -vs http://127.0.0.1:{self.ts.Variables.port}/drip?duration=4;
+curl -X PURGE --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin'].Variables.port}/drip?duration=4;
+timeout 2 curl --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin'].Variables.port}/drip?duration=4;
 sleep 4;
-curl --http1.1 -vs http://127.0.0.1:{self.ts.Variables.port}/drip?duration=4
+curl --http1.1 -vs http://127.0.0.1:{self.ts['for_httpbin'].Variables.port}/drip?duration=4
 """
         tr.Processes.Default.ReturnCode = 0
         tr.Processes.Default.Streams.stderr = "gold/background_fill_0_stderr.gold"
@@ -105,10 +127,10 @@ def __testCase1(self):
         tr = Test.AddTestRun()
         self.__checkProcessBefore(tr)
         tr.Processes.Default.Command = f"""
-curl -X PURGE --http1.1 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4;
-timeout 2 curl --http1.1 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4;
+curl -X PURGE --http1.1 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4;
+timeout 2 curl --http1.1 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4;
 sleep 4;
-curl --http1.1 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4
+curl --http1.1 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4
 """
         tr.Processes.Default.ReturnCode = 0
         tr.Processes.Default.Streams.stderr = "gold/background_fill_1_stderr.gold"
@@ -121,19 +143,36 @@ def __testCase2(self):
         tr = Test.AddTestRun()
         self.__checkProcessBefore(tr)
         tr.Processes.Default.Command = f"""
-curl -X PURGE --http2 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4;
-timeout 2 curl --http2 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4;
+curl -X PURGE --http2 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4;
+timeout 2 curl --http2 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4;
 sleep 4;
-curl --http2 -vsk https://127.0.0.1:{self.ts.Variables.ssl_port}/drip?duration=4
+curl --http2 -vsk https://127.0.0.1:{self.ts['for_httpbin'].Variables.ssl_port}/drip?duration=4
 """
         tr.Processes.Default.ReturnCode = 0
         tr.Processes.Default.Streams.stderr = "gold/background_fill_2_stderr.gold"
         self.__checkProcessAfter(tr)
 
+    def __testCase3(self):
+        """
+        HTTP/2 over TLS using ProxyVerifier
+        """
+        tr = Test.AddTestRun()
+        self.__checkProcessBefore(tr)
+        tr.AddVerifierClientProcess(
+            "pv_client",
+            "replay/bg_fill.yaml",
+            http_ports=[self.ts['for_pv'].Variables.port],
+            https_ports=[self.ts['for_pv'].Variables.ssl_port],
+            other_args='--thread-limit 1')
+        tr.Processes.Default.ReturnCode = 0
+        tr.Processes.Default.Streams.stdout = "gold/background_fill_3_stdout.gold"
+        self.__checkProcessAfter(tr)
+
     def run(self):
         self.__testCase0()
         self.__testCase1()
         self.__testCase2()
+        self.__testCase3()
 
 
 BackgroundFillTest().run()

tests/gold_tests/cache/gold/background_fill_3_stdout.gold

@@ -0,0 +1,15 @@
+``
+[DEBUG]: Received an HTTP/2 response for key 1 with stream id 1:
+:status: 200
+content-type: text/html
+content-length: 11
+``
+via: https/2 traffic_server (ApacheTrafficServer/`` [cMsSfW])
+``
+[DEBUG]: Received an HTTP/2 response for key 2 with stream id 1:
+:status: 200
+content-type: text/html
+content-length: 11
+``
+via: http/1.1 traffic_server (ApacheTrafficServer/`` [cHs f ])
+``

tests/gold_tests/cache/replay/bg_fill.yaml

@@ -0,0 +1,106 @@
+#  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 assumes that caching is enabled and
+# proxy.config.http.cache.ignore_client_cc_max_age is set to 0 so that we can
+# test max-age in the client requests.
+#
+
+
+meta:
+  version: '1.0'
+sessions:
+- protocol:
+  - name: http
+    version: 2
+  - name: tls
+    sni: test_sni
+  - name: tcp
+  - name: ip
+    version: 4
+  transactions:
+  - client-request:
+      frames:
+      - HEADERS:
+          headers:
+            fields:
+            - [:method, GET]
+            - [:scheme, https]
+            - [:authority, example.data.com]
+            - [:path, /a/path]
+            - [Content-Type, text/html]
+            - [uuid, 1]
+      - RST_STREAM:
+          delay: 1s
+          error-code: INTERNAL_ERROR
+
+    server-response:
+      frames:
+      - HEADERS:
+          headers:
+            fields:
+            - [:status, 200]
+            - [Content-Type, text/html]
+            - [Content-Length, '11']
+      - DATA:
+          delay: 2s
+          content:
+            encoding: plain
+            data: server_test
+            size: 11
+
+- protocol:
+  - name: http
+    version: 2
+  - name: tls
+    sni: test_sni
+  - name: tcp
+  - name: ip
+    version: 4
+  transactions:
+  - client-request:
+      delay: 3s
+      frames:
+      - HEADERS:
+          headers:
+            fields:
+            - [:method, GET]
+            - [:scheme, https]
+            - [:authority, example.data.com]
+            - [:path, /a/path]
+            - [Content-Type, text/html]
+            - [uuid, 2]
+
+    server-response:
+      frames:
+      - HEADERS:
+          headers:
+            fields:
+            - [:status, 200]
+            - [Content-Type, text/html]
+            - [Content-Length, '11']
+      - DATA:
+          content:
+            encoding: plain
+            data: server_test
+            size: 11
+
+    proxy-response:
+      content:
+        encoding: plain
+        data: server_test
+        verify: {as: equal}

tests/gold_tests/h2/gold/server_after_headers.gold

@@ -1,4 +1,4 @@
 ``
 ``Submitting RST_STREAM frame for key 1 after HEADERS frame with error code ENHANCE_YOUR_CALM.
-``Sent RST_STREAM frame for key 1 on stream 3.
+``Submitted RST_STREAM frame for key 1 on stream 3.
 ``