CDS: Fix member order in CdsApiImpl#15808
Conversation
The scope_ member is passed to subscription_ as a reference, so scope_ should live longer than subscription_, otherwise we risk a use of a dangling reference to scope_ inside subscription_. To make sure that scope_ lives longer than subscription_ order scope_ before subscription_. That way scope_ is constructed before subscription_ and destroyed after it. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
|
Hi @krnowak, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
@jmarantz can you take a quick look at this? At a high level this make sense to me and I'm OK without an explicit test (though it seems like we should be able to do it with mocking), but I'm curious why we haven't seen any issues before. I'm guessing it's because somehow we avoid issues with the global CDS and shutdown? |
jmarantz
left a comment
There was a problem hiding this comment.
Independent of whether there is a current crash, this change looks correct to me, and I'm inclined to approve it.
| void runInitializeCallbackIfAny(); | ||
|
|
||
| ClusterManager& cm_; | ||
| Stats::ScopePtr scope_; |
There was a problem hiding this comment.
Would be good to have a test to detect if member order ever regresses.
There was a problem hiding this comment.
Yes I agree. I think this should be possible with mocks per above. cc @krnowak if you could follow up and add this.
Commit Message:
The scope_ member is passed to subscription_ as a reference, so scope_
should live longer than subscription_, otherwise we risk a use of a
dangling reference to scope_ inside subscription_. To make sure that
scope_ lives longer than subscription_ order scope_ before
subscription_. That way scope_ is constructed before subscription_ and
destroyed after it.
Additional Description:
I found it tricky to write a test for this issue. I had crashes in
OdCdsApiImplwhich had the same issue (see the backtrace), but not withCdsApiImpl. The issue was quite involved (it was crashing on using an already-freed counter somewhere withinNewGrpcMuxImplbecause ofNewGrpcMuxImpl::WatchImplbegin destroyed (caused by destruction ofGrpcSubscriptionImpl). It does not look like a material for a unit test. Also, such a test could be flaky because of use-after-free being undefined behaviour.Risk Level:
Low.
Testing:
As a part of #15523.
Docs Changes:
N/A.
Release Notes:
N/A.
Platform Specific Features:
N/A.