-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
rds: workaround for the use-after-free crash. #3957
Conversation
This doesn't fix the underlying issue (that factory_context_ is being referenced after being freed), but it stops Envoy from crashing. *Risk Level*: Low *Testing*: bazel test //test/... (new test crashing without this patch) *Docs Changes*: n/a *Release Notes*: n/a Workaround for envoyproxy#3953. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
"2"); | ||
test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); | ||
|
||
// Update existing RDS (no changes). |
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.
If this is an update with changes, I think it still crash on https://github.com/PiotrSikora/envoy/blob/a5d68d41fb0323265302c04f40493c8a3d2a2c02/source/common/router/rds_impl.cc#L121
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.
You're right... I could swear that I tested this locally before, but it's indeed crashing there.
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.
Test added.
@@ -97,7 +98,7 @@ Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() { | |||
|
|||
void RdsRouteConfigProviderImpl::onConfigUpdate(const ResourceVector& resources, | |||
const std::string& version_info) { | |||
last_updated_ = factory_context_.systemTimeSource().currentTime(); | |||
last_updated_ = ProdSystemTimeSource::instance_.currentTime(); |
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.
Have a TimeSource& as member variable would be slightly better.
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.
Yeah, I need to mock it for the //test/common/router:rds_impl_test
anyway.
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Closing this, since @lizan is close having a proper fix |
This doesn't fix the underlying issue (that factory_context_ is being
referenced after being freed), but it stops Envoy from crashing.
Risk Level: Low
Testing: bazel test //test/... (new test crashing without this patch)
Docs Changes: n/a
Release Notes: n/a
Workaround for #3953.
Signed-off-by: Piotr Sikora piotrsikora@google.com