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

Fix potential null pointer deref. #103

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

jplevyak
Copy link

Signed-off-by: John Plevyak jplevyak@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@lizan
Copy link

lizan commented Sep 12, 2019

@jplevyak Great finding, can we do a full cherry-pick of envoyproxy#8106? which seems contains same change for warming clusters as well, and come with tests.

@duderino
Copy link

@lizan thanks for the tip. @jplevyak thanks for the PR.

Closing this in favor of the upstream cherry pick

@duderino duderino closed this Sep 12, 2019
@jplevyak
Copy link
Author

This doesn't cherrypick cleanly. My tiny patch is the same exact fix as in upstream minus the tests and other baggage. Do we want to do a more complex merge or go with the simpler fix?

@jplevyak jplevyak reopened this Sep 12, 2019
@lizan
Copy link

lizan commented Sep 12, 2019

We should at least also cherry-pick the regression test from upstream. The else branch doesn't do anything other than ASSERT but no hurt either.

@jplevyak
Copy link
Author

That is where the merge problems are. So full merge it is.

@jplevyak
Copy link
Author

This is 1.1 and there has been significant divergence. Consequentially there is no easy merge here. Several core objects have been altered. Getting the test to work would require substantial changes. Given that this change is completely safe (it only verifies that a pointer is not invalid before using it) and given that there are tests are in the other branches I think we should just proceed with the simple fix.

@lizan
Copy link

lizan commented Sep 13, 2019

@jplevyak sure that's fair.

@jplevyak jplevyak merged commit e5c31e3 into istio:release-1.1 Sep 13, 2019
Miss-you pushed a commit to Miss-you/envoy that referenced this pull request Nov 17, 2020
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.

5 participants