[GitHub] [trafficserver] bneradt commented on issue #9857: H2 POST request causes crash

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt commented on issue #9857:
URL: https://github.com/apache/trafficserver/issues/9857#issuecomment-1595028388

   I took a stab at reproducing this via an autest via the following patch:
   
   <details>
   <summary>Click to expand</summary>
   
   ```
   diff --git a/tests/gold_tests/h2/h2origin.test.py b/tests/gold_tests/h2/h2origin.test.py
   index cedbc7d00..e3db7b971 100644
   --- a/tests/gold_tests/h2/h2origin.test.py
   +++ b/tests/gold_tests/h2/h2origin.test.py
   @@ -17,6 +17,8 @@ Test communication to origin with H2
    #  See the License for the specific language governing permissions and
    #  limitations under the License.
    
   +import os
   +
    Test.Summary = '''
    Test communication to origin with H2
    '''
   @@ -32,6 +34,7 @@ ts = Test.MakeATSProcess("ts", enable_tls="true")
    ts.addDefaultSSLFiles()
    replay_file = "replay_h2origin/"
    server = Test.MakeVerifierServerProcess("h2-origin", replay_file)
   +httpbin = Test.MakeHttpBinServer("httpbin")
    ts.Disk.records_config.update({
        'proxy.config.ssl.server.cert.path': '{0}'.format(ts.Variables.SSLDir),
        'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir),
   @@ -47,9 +50,10 @@ ts.Disk.records_config.update({
        'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
    })
    
   -ts.Disk.remap_config.AddLine(
   -    'map / https://127.0.0.1:{0}'.format(server.Variables.https_port)
   -)
   +ts.Disk.remap_config.AddLines([
   +    f'map /httpbin http://127.0.0.1:{httpbin.Variables.Port}',
   +    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'
    )
   @@ -74,14 +78,22 @@ tr.AddVerifierClientProcess("client", replay_file, http_ports=[ts.Variables.port
    tr.StillRunningAfter = ts
    tr.TimeOut = 60
    
   -# Just a check to flush out the traffic log until we have a clean shutdown for traffic_server
   -tr = Test.AddTestRun("Wait for the access log to write out")
   -tr.DelayStart = 10
   -tr.StillRunningAfter = ts
   -tr.StillRunningAfter = server
   -tr.Processes.Default.Command = 'ls'
   +tr = Test.AddTestRun("Test a POST request with an Expect: 100-continue header")
   +content_file = os.path.join(Test.RunDirectory, '8k')
   +with open(content_file, 'wb') as f:
   +    f.write(os.urandom(8192))
   +tr.Processes.Default.StartBefore(httpbin)
   +tr.Processes.Default.Command = (
   +    f'curl -kv -d @{content_file} --http2 -H "Expect: 100-Continue" '
   +    f'https://127.0.0.1:{ts.Variables.ssl_port}/httpbin/post')
    tr.Processes.Default.ReturnCode = 0
    
   +# Wait until the transaction log is written.
   +tr = Test.AddTestRun("Wait for the access log to write out")
   +cond_wait_path = os.path.join(Test.Variables.AtsTestToolsDir, 'condwait')
   +squid_log_path = os.path.join(ts.Variables.LOGDIR, 'squid.log')
   +tr.Processes.Default.Command = f'{cond_wait_path} 60 1 -f {squid_log_path}'
   +
    # UUIDs 1-4 should be http/1.1 clients and H2 origin
    # UUIDs 5-9 should be http/2 clients and H2 origins
    ts.Disk.squid_log.Content = Testers.ContainsExpression(" [1-4] http/1.1 http/2", "cases 1-4 request http/1.1")
   ```
   </details>
   
   The interesting portion of the patch being the following new TestRun:
   
   ```python3
   tr = Test.AddTestRun("Test a POST request with an Expect: 100-continue header")
   content_file = os.path.join(Test.RunDirectory, '8k')
   with open(content_file, 'wb') as f:
       f.write(os.urandom(8192))
   tr.Processes.Default.StartBefore(httpbin)
   tr.Processes.Default.Command = (
       f'curl -kv -d @{content_file} --http2 -H "Expect: 100-Continue" '
       f'https://127.0.0.1:{ts.Variables.ssl_port}/httpbin/post')
   tr.Processes.Default.ReturnCode = 0
   ```
   
   Our local autests use go-httpbin because httpbin is deprecated and no longer supported. Masakazu confirmed that when he switched his remap from `map /httpbin/ http://httpbin.org/` to `map /httpbin/ http://httpbingo.org/`, the crash no longer reproduced. So it seems the crash relies upon some behavior in httpbin that go-httpbin does not quite replicate.
   
   I'm going to try to reproduce the crash using Proxy Verifier. I think I saw a crash with that in an earlier attempt, but Proxy Verifier could use some `Expect: 100-continue` updates.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org