Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid chunking in the post redirect case #8268

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

duke8253
Copy link
Contributor

After fixing #8265 we started seeing some crashes happening, this fixes the crash.

@duke8253 duke8253 added this to the 10.0.0 milestone Aug 20, 2021
@duke8253 duke8253 requested a review from shinrich August 20, 2021 16:00
@duke8253 duke8253 requested a review from zwoop as a code owner August 20, 2021 16:00
@duke8253 duke8253 self-assigned this Aug 20, 2021
@shinrich
Copy link
Member

The problem is we were marking that the post body should be chunked. But in the post redirect case, the data is being replayed from a static buffer. There is no real VC to be doing the chunked calculation against. So p->do_chunking is not set. So chunked_buffer_start is not set. So the call to clone that nullptr causes the crash.

[ 00 ] traffic_server      HttpTunnel::producer_run(HttpTunnelProducer*)                ( P_IOBuffer.h:753 )
[ 01 ] traffic_server      HttpTunnel::tunnel_run(HttpTunnelProducer*)                  ( HttpTunnel.cc:720 )
[ 02 ] traffic_server      HttpSM::do_setup_post_tunnel(HttpVC_t)                       ( HttpSM.cc:5923 )
[ 03 ] traffic_server      HttpSM::state_send_server_request_header(int, void*)         ( HttpSM.cc:2197 )
[ 04 ] traffic_server      HttpSM::state_read_server_response_header(int, void*)        ( HttpSM.cc:2004 )
[ 05 ] traffic_server      HttpSM::main_handler(int, void*)                             ( HttpSM.cc:2701 )
[ 06 ] traffic_server      write_signal_and_update(int, UnixNetVConnection*)            ( UnixNetVConnection.cc:117 )
[ 07 ] traffic_server      write_to_net_io(NetHandler*, UnixNetVConnection*, EThread*)  ( UnixNetVConnection.cc:162 )
[ 08 ] traffic_server      NetHandler::process_ready_list()                             ( UnixNet.cc:432 )
[ 09 ] traffic_server      NetHandler::waitForActivity(long)                            ( UnixNet.cc:552 )
[ 10 ] traffic_server      EThread::execute_regular()                                   ( UnixEThread.cc:303 )
[ 11 ] traffic_server      execute                                                      ( UnixEThread.cc:364 )
[ 12 ] traffic_server      EThread::execute()                                           ( UnixEThread.cc:342 )
[ 13 ] traffic_server      spawn_thread_internal                                        ( Thread.cc:92 )
[ 14 ] libpthread-2.17.so  start_thread                                                 ( pthread_create.c:307 )

I think in the past (9.0), we were placing transfer-encoding headers on the H2 client requests. And chunkifying the H2 request data before it came to the HttpSM. With 9.1, we cleaned up that logic.

@bryancall bryancall requested a review from randall August 23, 2021 23:08
@masaori335
Copy link
Contributor

Any idea of why this AuTest doesn't cover this case? Can we make it to cover?

# Test Case 6: Post with chunked body
# While HTTP/2 does not support Transfer-encoding we pass that into curl to encourage it to not set the content length
# on the post body
tr = Test.AddTestRun()
tr.Processes.Default.Command = 'curl -s -k -H "Transfer-Encoding: chunked" -d "{0}" https://127.0.0.1:{1}/postchunked'.format(
post_body, ts.Variables.ssl_port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = "gold/post_chunked.gold"
tr.StillRunningAfter = server

@shinrich
Copy link
Member

We can add a test for redirects with a chunked post. Perhaps in a separate PR. I don't think the existing tests have follow redirect enabled.

@masaori335
Copy link
Contributor

Right, we haven't covered the combination of POST + Chunk + Redirect + H2. Separate PR is good. Thanks!

@duke8253 duke8253 merged commit cd1139b into apache:master Sep 27, 2021
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 7, 2022
* asf/master: (47 commits)
  Doc: cleanup build errors. (apache#8377)
  Doc: Add proxy.config.cacvhe.mutex_retry_delay (apache#8376)
  Adds support for TCP_NOTSENT_LOWAT sockopt (apache#8354)
  Added support for promoting internal (plugin-initiated) requests. (apache#8363)
  Removed references to the throttle option from the slice plugin. (apache#8373)
  Pre-warming TLS Tunnel (apache#7661)
  Traffic Dump: update json-schema for new tuple requirements (apache#8370)
  TSSslSecretSet: Update SSL_CTX TLS Secrets (apache#8368)
  Added support for verifying cacheability before attempting to force an object into cache (apache#8364)
  [doc] Add a note for TSLifecycleHookAdd. Warn users that a contp could eventually be executed in a ET_NET when it was originally scheduled in the ET_TASK. (apache#8344)
  check size of session, and free sessions the ATS way (apache#8330)
  Locking around SSLSecret::secret_map access (apache#8358)
  Stabilize regex_revalidate Au test. (apache#559) (apache#8360)
  change MemArena::make test to remove memory leak (apache#8352)
  free sessions when timeout (apache#8356)
  test_MMH: fix memory leak in unit test (apache#8357)
  crash fix (apache#8268)
  remove unused RecConfigFileEntry struct (apache#8353)
  Rename outbound_conntrack to global_outbound_conntrack to reduce confusion. (apache#8343)
  remove unused RecConfigFileEntry from RecConfigParse (apache#8348)
  ...
@bryancall bryancall added the Crash label Aug 8, 2022
zwoop pushed a commit that referenced this pull request Aug 10, 2022
(cherry picked from commit cd1139b)
@zwoop
Copy link
Contributor

zwoop commented Aug 10, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Aug 10, 2022
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
* asf/9.2.x:
  Updated ChangeLog
  Fix length bug in validate_unmapped_url_path (apache#8080)
  crash fix (apache#8268)
  test_MMH: fix memory leak in unit test (apache#8357)
  Doc: Add proxy.config.cacvhe.mutex_retry_delay (apache#8376)
  Add thread safety to PendingAction operations. (apache#8443)
  Report an error if configure can't find zlib (apache#8446)
  Update roadmap doc with latest releases (apache#8977)
  Setup UA consumer only if ua_entry is not nullptr (apache#8949)
  Update slice to only prefetch when first block is miss/hit-stale (apache#8890)
  Add RangeTransform::m_write_vio state checks (apache#8980)
  Fix compile on M1 Mac (apache#8999)
  Add stack guard pages (apache#8996)
  Fail fast on HTTP/2 header validation (apache#9009)
  Restrict unknown scheme of HTTP/2 request (apache#9010)
  Add content length mismatch check on handling HEADERS frame and CONTINUATION frame (apache#9012)
  Ignore POST request case from a check for background fill (apache#9013)
  Add back validatation that the scheme matches the wire protocol (apache#9005)
  Pin flask to version 2.1.3 (apache#9008)
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 19, 2023
* add a metric to track how often the range seek bug is detected (apache#8970)

Co-authored-by: Chris McFarlen <cmcfarlen@apple.com>
(cherry picked from commit b23e8a0)

* Fix reverting PR#7302 (apache#8975)

PR#7302 was reverted by PR#8316 as an incompatible change for 9.2.0.
It looks like the revert commit has a mistake that made a crash by
calling `HttpSM::send_origin_throttled_response()` twice.

(cherry picked from commit 3cccd2d)

* Fixes issue with file size calculation for existing logs (apache#8971)

* Issue arises with existing log files at startup

* Because the existing bytes are not accounted for, log rolling does not occur at the correct time

* Existing code can lead to logging being suspended indefinitely without manual intervention if thresholds are exceeded and no rolled log files can be deleted

* Corner case more evident when other data not rolled by ATS is present in the logging directory

(cherry picked from commit 6225b12)

* Proxy Verifier: Update to version 2.4.1 (apache#8965)

This updates the Proxy Verifier version used by our AuTests to version
v2.4.1. This Proxy Verifier version prints the ALPN used by the proxy
(ATS in our case) to the server. This will allow us to verify ALPN
behavior for an upcoming PR for HTTP/2 to origin.

(cherry picked from commit fb02ef8)

* Destroy ssl context after use. (apache#8531)

As per the docs this needs to be released after use, this was missing from the cert_reporting_tool plugin.
This also fixes the example in the docs.

(cherry picked from commit 57015b7)

* Extend milestone api time tracking to remap. (apache#8520)

(cherry picked from commit a9405ac)

* Add 5xx's to be allowed to be used for simple retries (apache#8518)

* Add 5xx's to be allowed to be used for simple retries

Remove unnecessary functions in transact for finding ranges

Change PS response checking to not use internal state. Now pass in retry type and code

(cherry picked from commit 30096b4)

* Updated ChangeLog

* Pin flask to version 2.1.3 (apache#9008)

This resolves an AuTest Pipenv package dependency conflict for Werkzeug,
which is used by httpbin. Latest versions of flask require newer
versions of flask which conflicts with our pin to keep httpbin working.

(cherry picked from commit 46c1a0a)

* Add back validatation that the scheme matches the wire protocol (apache#9005)

This adds back in the scheme and wire protocol check (see apache#8465) along
with a configuration to be able to disable the check if the verification
is not desired.

(cherry picked from commit 7ec147e)

* Ignore POST request case from a check for background fill (apache#9013)

(cherry picked from commit 1f3e111)

* Add content length mismatch check on handling HEADERS frame and CONTINUATION frame (apache#9012)

* Add content length mismatch check on handling HEADERS frame and CONTINUATION frame

* Correct error class of HTTP/2 malformed requests

(cherry picked from commit e921228)

* Restrict unknown scheme of HTTP/2 request (apache#9010)

Strictly following RFC 3986 Section 3.1

```
scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
```

(cherry picked from commit c56f872)

* Fail fast on HTTP/2 header validation (apache#9009)

Co-authored-by: Masakazu Kitajo <maskit@apache.org>
(cherry picked from commit eaef5e8)

* Add stack guard pages (apache#8996)

Use r/w protected pages on top of stacks to guard against stack
overflow.  The number of VM pages to use for guarding can be set
via the config option proxy.config.thread.default.stackguard_pages.

(cherry picked from commit 1abf6c0)

* Fix compile on M1 Mac (apache#8999)

Add arm64 to the list of known stack growth directions.

(cherry picked from commit 697da39)

* Add RangeTransform::m_write_vio state checks (apache#8980)

(cherry picked from commit e912ece)

* Update slice to only prefetch when first block is miss/hit-stale (apache#8890)

* Update slice to only prefetch when first block is miss/hit-stale

* Remove extra line spaces & generalize autest output

* Verify cont is valid

* Use xdebug to only prefetch when first block is cacheable with miss/hit-stale status

* Precompile via regex pattern in config

* Remove plugin dependency, add cache status header between slice and crr

* Only enable prefetching from CRR on 206 partial case from origin

* Update header type, fix 206 case for 304 in CRR, allow header to be used for debugging

* update header val type

Co-authored-by: Serris Lew <lserris@apple.com>
(cherry picked from commit f14cce4)

 Conflicts:
	doc/admin-guide/plugins/slice.en.rst

* Setup UA consumer only if ua_entry is not nullptr (apache#8949)

(cherry picked from commit cbe0bea)

* Update roadmap doc with latest releases (apache#8977)

(cherry picked from commit 80a0ff9)

* Report an error if configure can't find zlib (apache#8446)

(cherry picked from commit a100761)

* Add thread safety to PendingAction operations. (apache#8443)

(cherry picked from commit 29a5092)

* Doc: Add proxy.config.cacvhe.mutex_retry_delay (apache#8376)

(cherry picked from commit 3ad1587)

* test_MMH: fix memory leak in unit test (apache#8357)

(cherry picked from commit 0eccef0)

* crash fix (apache#8268)

(cherry picked from commit cd1139b)

* Fix length bug in validate_unmapped_url_path (apache#8080)

(cherry picked from commit ac16a3b)

* Updated ChangeLog

Co-authored-by: Chris McFarlen <chris@mcfarlen.us>
Co-authored-by: Masaori Koshiba <masaori@apache.org>
Co-authored-by: Jeff Elsloo <elsloo@users.noreply.github.com>
Co-authored-by: Brian Neradt <brian.neradt@gmail.com>
Co-authored-by: Damian Meden <damian.meden@gmail.com>
Co-authored-by: Alan M. Carroll <amc@apache.org>
Co-authored-by: Evan Zelkowitz <eze@apache.org>
Co-authored-by: Leif Hedstrom <zwoop@apache.org>
Co-authored-by: Mo Chen <uncorrupt@gmail.com>
Co-authored-by: Serris Lew <serrisnlew@gmail.com>
Co-authored-by: Matt Williams <gh@mattyw.net>
Co-authored-by: Bryan Call <bcall@apache.org>
Co-authored-by: Brian Olsen <bnolsen@gmail.com>
Co-authored-by: Fei Deng <duke8253@gmail.com>
Co-authored-by: bneradt <bneradt@yahooinc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants