Update QUICHE dependency by DavidSchinazi · Pull Request #16100 · envoyproxy/envoy

DavidSchinazi · 2021-04-21T15:47:56Z

This PR updates our QUICHE dependency to commit
88c8d5903d851744410ea9840201b6507feae981.

repokitteh-read-only · 2021-04-21T15:48:02Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16100 was opened by DavidSchinazi.

see: more, trace.

DavidSchinazi · 2021-04-21T15:48:12Z

/assign @danzh1989

repokitteh-read-only · 2021-04-21T15:48:15Z

@danzh1989 cannot be assigned to this issue.

🐱

Caused by: a #16100 (comment) was created by @DavidSchinazi.

see: more, trace.

This PR updates our QUICHE dependency to commit 88c8d5903d851744410ea9840201b6507feae981. Signed-off-by: David Schinazi <dschinazi@google.com>

DavidSchinazi · 2021-04-21T16:09:43Z

/assign @danzh1989

repokitteh-read-only · 2021-04-21T16:09:46Z

@danzh1989 cannot be assigned to this issue.

🐱

Caused by: a #16100 (comment) was created by @DavidSchinazi.

see: more, trace.

DavidSchinazi · 2021-04-21T16:11:29Z

/assign @danzh1989

repokitteh-read-only · 2021-04-21T16:11:32Z

@danzh1989 cannot be assigned to this issue.

🐱

Caused by: a #16100 (comment) was created by @DavidSchinazi.

see: more, trace.

moderation

Otherwise deps look OK

bazel/repository_locations.bzl

Signed-off-by: David Schinazi <dschinazi@google.com>

danzh2010

Thanks for helping with updating quiche deps!

bazel/external/quiche.BUILD

source/common/quic/quic_filter_manager_connection_impl.cc

Signed-off-by: David Schinazi <dschinazi@google.com>

danzh2010

LGTM! Plz fix the DCO.

test/integration/multiplexed_upstream_integration_test.cc

Signed-off-by: David Schinazi <dschinazi@google.com>

danzh2010 · 2021-04-22T17:50:15Z

/assign @htuch

moderation · 2021-04-22T18:45:06Z

/lgtm deps

htuch

LGTM, thanks!

moderation · 2021-04-23T15:17:01Z

@danzh2010 @DavidSchinazi @alyssawilk I'm able to crash Envoy after this update. Using Firefox nightly with a downstream h3 config I can now generate terminated by signal SIGSEGV (Address boundary error) within a couple of page reloads.

[2021-04-23 08:10:42.986][127974][error][envoy_bug] [source/common/quic/envoy_quic_server_session.cc:50] envoy bug failure: false. Details: Quic session 3175632122628430394 attempts to create stream 0 before HCM filter is initialized.

Looks like only Firefox nightly triggers this. The other h3 clients are all working fine - https://dev.to/moderation/experiments-with-h3-clients-envoy-52gj

danzh1989 · 2021-04-23T15:38:31Z

Thanks for reporting this crash! HCM filter is initialized after handshake complete. My guess is that this update enables 0-RTT, which creates streams before handshake complete. #15912 would fix the issue. But meanwhile, we probably should disable 0-RTT support in the QUIC listener.

…

On Fri, Apr 23, 2021 at 11:17 AM moderation ***@***.***> wrote: @danzh2010 <https://github.com/danzh2010> @DavidSchinazi <https://github.com/DavidSchinazi> @alyssawilk <https://github.com/alyssawilk> I'm able to crash Envoy after this update. Using Firefox nightly with a downstream h3 config I can now generate terminated by signal SIGSEGV (Address boundary error) within a couple of page reloads. [2021-04-23 08:10:42.986][127974][error][envoy_bug] [source/common/quic/envoy_quic_server_session.cc:50] envoy bug failure: false. Details: Quic session 3175632122628430394 attempts to create stream 0 before HCM filter is initialized. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADQ23C6EUT6EJLNNYCQBAXLTKGFQDANCNFSM43KUCNZQ> .

danzh1989 · 2021-04-23T15:41:20Z

Michael, can you also provide the crash stack?

…

On Fri, Apr 23, 2021 at 11:38 AM Dan Zhang ***@***.***> wrote: Thanks for reporting this crash! HCM filter is initialized after handshake complete. My guess is that this update enables 0-RTT, which creates streams before handshake complete. #15912 would fix the issue. But meanwhile, we probably should disable 0-RTT support in the QUIC listener. On Fri, Apr 23, 2021 at 11:17 AM moderation ***@***.***> wrote: > @danzh2010 <https://github.com/danzh2010> @DavidSchinazi > <https://github.com/DavidSchinazi> @alyssawilk > <https://github.com/alyssawilk> I'm able to crash Envoy after this > update. Using Firefox nightly with a downstream h3 config I can now > generate terminated by signal SIGSEGV (Address boundary error) within a > couple of page reloads. > > [2021-04-23 08:10:42.986][127974][error][envoy_bug] > [source/common/quic/envoy_quic_server_session.cc:50] envoy bug failure: > false. Details: Quic session 3175632122628430394 attempts to create stream > 0 before HCM filter is initialized. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#16100 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADQ23C6EUT6EJLNNYCQBAXLTKGFQDANCNFSM43KUCNZQ> > . >

alyssawilk · 2021-04-26T14:13:28Z

Can we just flag off 0-rtt for both upstream and downstream? I assume there's a flag for that and neither side appears to support it yet.

DavidSchinazi · 2021-04-26T14:26:31Z

The crash discussed above should be resolved by #16147

danzh1989 · 2021-04-26T18:03:55Z

I suspect we already support 0-RTT upstream. The resumption ticket is stored in client crypto config, so as long as our client reuse it upstream client will get 0-RTT. But I need to test and verify that.

…

On Mon, Apr 26, 2021 at 10:26 AM David Schinazi ***@***.***> wrote: The crash discussed above should be resolved by #16147 <#16147> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADQ23C6LQ7UFFKEKDAWCYO3TKVZ2TANCNFSM43KUCNZQ> .

moderation · 2021-04-26T22:52:05Z

I can report that the crash still happens after #16147. To reliably reproduce I can pull up a listener in Firefox Nightly on Linux and load the page. On first load and subsequent immediate reloads the site is fine. But then if I step away for > 5 mins and reload, crash.

This PR updates our QUICHE dependency to commit 88c8d5903d851744410ea9840201b6507feae981. Signed-off-by: David Schinazi <dschinazi@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 21, 2021

Update QUICHE dependency

51bd0da

This PR updates our QUICHE dependency to commit 88c8d5903d851744410ea9840201b6507feae981. Signed-off-by: David Schinazi <dschinazi@google.com>

DavidSchinazi force-pushed the quiche_merge_20210421 branch from a69206e to 51bd0da Compare April 21, 2021 15:55

moderation suggested changes Apr 21, 2021

View reviewed changes

bazel/repository_locations.bzl Outdated Show resolved Hide resolved

fix comment

df5fcaa

Signed-off-by: David Schinazi <dschinazi@google.com>

danzh2010 reviewed Apr 21, 2021

View reviewed changes

DavidSchinazi added 2 commits April 21, 2021 20:03

review comments

8eb252a

Signed-off-by: David Schinazi <dschinazi@google.com>

enable fixed tests

8ab30b7

Signed-off-by: David Schinazi <dschinazi@google.com>

asraa assigned danzh2010 Apr 21, 2021

danzh2010 previously approved these changes Apr 22, 2021

View reviewed changes

test/integration/multiplexed_upstream_integration_test.cc Outdated Show resolved Hide resolved

re-enable other fixed tests

83e82f9

Signed-off-by: David Schinazi <dschinazi@google.com>

DavidSchinazi dismissed danzh2010’s stale review via 83e82f9 April 22, 2021 15:17

DavidSchinazi force-pushed the quiche_merge_20210421 branch from bdd3571 to 83e82f9 Compare April 22, 2021 15:17

fix format

917a22a

Signed-off-by: David Schinazi <dschinazi@google.com>

repokitteh-read-only bot assigned htuch Apr 22, 2021

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 22, 2021

htuch approved these changes Apr 23, 2021

View reviewed changes

htuch merged commit c0c243e into envoyproxy:main Apr 23, 2021

alyssawilk mentioned this pull request Apr 29, 2021

regression test QUIC crash caused by the import #16234

Closed

DavidSchinazi deleted the quiche_merge_20210421 branch January 27, 2023 02:07

Conversation

DavidSchinazi commented Apr 21, 2021

Uh oh!

repokitteh-read-only bot commented Apr 21, 2021

Uh oh!

DavidSchinazi commented Apr 21, 2021

Uh oh!

repokitteh-read-only bot commented Apr 21, 2021

Uh oh!

DavidSchinazi commented Apr 21, 2021

Uh oh!

repokitteh-read-only bot commented Apr 21, 2021

Uh oh!

DavidSchinazi commented Apr 21, 2021

Uh oh!

repokitteh-read-only bot commented Apr 21, 2021

Uh oh!

moderation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danzh2010 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danzh2010 commented Apr 22, 2021

Uh oh!

moderation commented Apr 22, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

moderation commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh1989 commented Apr 23, 2021 via email

Uh oh!

danzh1989 commented Apr 23, 2021 via email

Uh oh!

alyssawilk commented Apr 26, 2021

Uh oh!

DavidSchinazi commented Apr 26, 2021

Uh oh!

danzh1989 commented Apr 26, 2021 via email

Uh oh!

moderation commented Apr 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danzh2010 left a comment •

edited

Loading

moderation commented Apr 23, 2021 •

edited

Loading