-
Notifications
You must be signed in to change notification settings - Fork 60
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
Merge pull request #111 from jplevyak/release-1.3-use-after-free #113
Merge pull request #111 from jplevyak/release-1.3-use-after-free #113
Conversation
Apply fix for use-after-free in Envoy ThreadLocal Slot. Signed-off-by: John Plevyak <jplevyak@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'll let an Envoy specialist approve this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as it's almost equivalent to the 1.2 PR
@@ -280,8 +280,12 @@ class MutableConfigProviderImplBase : public ConfigProvider { | |||
* @param config supplies the newly instantiated config. | |||
*/ | |||
void onConfigUpdate(const ConfigConstSharedPtr& config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a if (getConfig() == config) { return; } in 1.2
Add to 1.1. as well ?
The code it was replacing didn't do that check in this version. Do you
want me to add it?
…On Fri, Oct 11, 2019, 11:54 AM Yuchen Dai ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM as it's almost equivalent to the 1.2 PR
------------------------------
In source/common/config/config_provider_impl.h
<#113 (comment)>:
> @@ -280,8 +280,12 @@ class MutableConfigProviderImplBase : public ConfigProvider {
* @param config supplies the newly instantiated config.
*/
void onConfigUpdate(const ConfigConstSharedPtr& config) {
I see a if (getConfig() == config) { return; } in 1.2
Add to 1.1. as well ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#113?email_source=notifications&email_token=AACEXIAWPGQ5KKEMKFNIYL3QODDX7A5CNFSM4I7VRL42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHXORSA#pullrequestreview-300869832>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACEXIADEOWYURHSVFWNUKTQODDX7ANCNFSM4I7VRL4Q>
.
|
Both are ok. I won't be worry about it |
This is similar to the http2 frame protection, but rather than try to guard [header block || last body bytes || last chunk in chunk encoding || trailer block] depending on end stream, which just gets messy, I opted to just add an empty reference counted fragment after the body was serialized, which appears to work just as well with a small theoretical overhead. If folks think the complexity is warranted I can of course do that instead. Risk Level: Medium Testing: new unit tests, integration test Docs Changes: stats documented Release Notes: added Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Jianfei Hu <jianfeih@google.com>
This is similar to the http2 frame protection, but rather than try to guard [header block || last body bytes || last chunk in chunk encoding || trailer block] depending on end stream, which just gets messy, I opted to just add an empty reference counted fragment after the body was serialized, which appears to work just as well with a small theoretical overhead. If folks think the complexity is warranted I can of course do that instead. Risk Level: Medium Testing: new unit tests, integration test Docs Changes: stats documented Release Notes: added Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Co-authored-by: Lizan Zhou <lizan@tetrate.io>
This is similar to the http2 frame protection, but rather than try to guard [header block || last body bytes || last chunk in chunk encoding || trailer block] depending on end stream, which just gets messy, I opted to just add an empty reference counted fragment after the body was serialized, which appears to work just as well with a small theoretical overhead. If folks think the complexity is warranted I can of course do that instead. Risk Level: Medium Testing: new unit tests, integration test Docs Changes: stats documented Release Notes: added Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This is similar to the http2 frame protection, but rather than try to guard [header block || last body bytes || last chunk in chunk encoding || trailer block] depending on end stream, which just gets messy, I opted to just add an empty reference counted fragment after the body was serialized, which appears to work just as well with a small theoretical overhead. If folks think the complexity is warranted I can of course do that instead. Risk Level: Medium Testing: new unit tests, integration test Docs Changes: stats documented Release Notes: added Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Jianfei Hu <jianfeih@google.com>
zh translation intro\arch_overview\listeners\listeners_toc.rst.
Apply fix for use-after-free in Envoy ThreadLocal Slot.
Signed-off-by: John Plevyak jplevyak@gmail.com