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

[http2] Dont drop connections on metadata limit exceeded #32309

Merged
merged 4 commits into from Feb 7, 2023

Conversation

ctiller
Copy link
Member

@ctiller ctiller commented Feb 7, 2023

If we get an error on a stream, all we need to do is cancel that stream.
If instead we get an error not associated with a stream, we should close the transport.

Current behavior was to just close the transport always, and this was a bad bug.

@ctiller ctiller requested a review from yashykt February 7, 2023 07:36
@ctiller ctiller added the release notes: no Indicates if PR should not be in release notes label Feb 7, 2023
@ctiller ctiller requested a review from soheilhy February 7, 2023 07:36
@ctiller ctiller changed the title [http] Dont drop connections on metadata limit exceeded [http2] Dont drop connections on metadata limit exceeded Feb 7, 2023
grpc_call_unref(s);

grpc_slice_unref(meta.value);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also do a ping to make sure that the channel is still connected?

Automated fix for refs/heads/repeated_large_metadata
@ctiller ctiller merged commit 29d8bee into grpc:master Feb 7, 2023
@ctiller ctiller deleted the repeated_large_metadata branch February 7, 2023 16:27
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 7, 2023
@ctiller ctiller added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Feb 8, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
* [http] Dont drop connections on metadata limit exceeded

* remove bad test

* Automated change: Fix sanity tests

---------

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
* [http] Dont drop connections on metadata limit exceeded

* remove bad test

* Automated change: Fix sanity tests

---------

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
jpuhlman pushed a commit to MontaVista-OpenSourceTechnology/meta-openembedded that referenced this pull request Jul 24, 2023
Source: meta-openembedded
MR: 126092
Type: Integration
Disposition: Merged from meta-openembedded
ChangeID: d7ece8b
Description:

It was introduced in in v1.53.0 and not backported to v1.46.x branch.
NVD references PR which intrioduces the vulnerability:
  grpc/grpc#32309 (comment)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Armin Kuster <akuster808@gmail.com>
Signed-off-by: Jeremy A. Puhlman <jpuhlman@mvista.com>
@ChenQi1989
Copy link

@ctiller Sorry, but I'm a little confused.
https://nvd.nist.gov/vuln/detail/CVE-2023-32732 says:
"""
We recommend upgrading beyond the commit in #32309
"""
But looking at the above comments, it seems that PR#32309 is not a fix, instead, it's the cause of CVE-2023-32732.

Which one is correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants