Skip to content

server: fix ~InstanceImpl() segfault #4244

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.

Sign up for GitHub

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

Merged
htuch merged 1 commit into from
Aug 30, 2018
Merged

server: fix ~InstanceImpl() segfault #4244

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,17 @@ InstanceImpl::~InstanceImpl() {
// Stop logging to file before all the AccessLogManager and its dependencies are
// destructed to avoid crashing at shutdown.
file_logger_.reset();

// Destruct the ListenerManager explicitly, before InstanceImpl's local init_manager_ is
// destructed.
//
// The ListenerManager's DestinationPortsMap contains FilterChainSharedPtrs. There is a rare race
Copy link
Member

@lizan lizan Aug 24, 2018

Choose a reason for hiding this comment

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

I'm not sure how this is a RARE race, can you elaborate a bit more? Particularly, when RdsRouteConfigSubscription get registered to top-level InitManager?

  • FilterChains contains HttpConnectionManager is pretty common
  • HttpConnectionManager contains RdsRouteConfigProvider is pretty common too
  • Any dynamic RdsRouteConfigProvider contains a RdsRouteConfigSubscription

Copy link
Contributor Author

@ambuc ambuc Aug 24, 2018

Choose a reason for hiding this comment

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

I think the circumstances are common but the resulting segfault is rare.

I'm not 100% on how memory works here under the hood, but my personal theory is that when the subscription goes to unregister itself from the list of targets_ inside InitManagerImpl, targets_ has just been freed but not not overwritten (?) so this operation often succeeds. @mrice32 might have more color here.

Copy link
Member

@lizan lizan Aug 24, 2018

Choose a reason for hiding this comment

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

Yes use-after-free is always rare without ASAN, does that mean you can have a config which always trigger use-after-free with ASAN?

Copy link
Contributor Author

@ambuc ambuc Aug 24, 2018

Choose a reason for hiding this comment

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

That's an interesting thought, the answer is surely yes. I'll do some digging and report back.

Copy link
Member

@lizan lizan Aug 24, 2018

Choose a reason for hiding this comment

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

Once you have that, we could have some sort of test here. Could be an integration test with shutdown or sth similar.

// condition where one of these FilterChains contains an HttpConnectionManager, which contains an
// RdsRouteConfigProvider, which contains an RdsRouteConfigSubscriptionSharedPtr. Since
// RdsRouteConfigSubscription is an Init::Target, ~RdsRouteConfigSubscription triggers a callback
// set at initialization, which goes to unregister it from the top-level InitManager, which has
// already been destructed (use-after-free) causing a segfault.
listener_manager_.reset();
Copy link
Member

@lizan lizan Aug 24, 2018

Choose a reason for hiding this comment

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

While I think this did fix for the case described above, can we move init_manager_ before listener_manager or potentially anything may contain it, in member variable list? InitManagerImpl doesn't seems to have anything depends on others, so it could moved before other stuff.

Copy link
Contributor Author

@ambuc ambuc Aug 24, 2018

Choose a reason for hiding this comment

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

I'm happy to move the private member variables around, but without a test it's easy to imagine this accidentally getting undone in the future. A manual .reset() seems like a better solution for that fear.

}

Upstream::ClusterManager& InstanceImpl::clusterManager() { return *config_->clusterManager(); }
Expand Down