server: fix use-after-free in InitManagerImpl #6136
Conversation
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
There was a problem hiding this comment.
Thanks for iterating on this.
I'm still not crazy about this solution and would prefer some type of handle which when destructed performs a cancellation, but I'm not going to fight for it a lot. If we keep this solution, can we add more comments in each place where canceled_ is interacted with? @lizan any thoughts here?
/wait
Same here, I thought f47a376 was very close but just need a little more fix for mocks, no? |
|
@mattklein123 @lizan - I'm actually working on a fairly different approach right now that should be more robust in all cases... It's a bigger change; I'll ping you when that's ready. |
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
|
Alright, I'm following @lizan's advice... I'm sticking with the |
|
@mergeconflict can you look at CI? Those look like legit files that might be due to your change. Thank you. /wait |
Yeah, I saw... I'm sure it's more breakage caused by my changes. I'll keep working on this tomorrow. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Introduce a new "safe" init manager, to replace the existing one that's prone to use-after-free issues (see e.g. #6116). Users of the existing init manager will be upgraded one-by-one in subsequent PRs if this design is approved. See also previous false starts in PRs #6136 and #6245. Risk Level: Low, no existing users of the existing init manager are changed in this PR. Testing: New unit tests added. Docs Changes: n/a Release Notes: n/a Signed-off-by: Dan Rosen <mergeconflict@google.com>
Description: Fix use-after-free where InitManagerImpl is deleted before its registered targets are finished initializing. Fixes #6116.
Risk Level: Low
Testing: Added ADS integration test which reproduces the crash without the fix.
Docs Changes: n/a
Release Notes: n/a