Fix segfault caused by modifying list within iteration. #922
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A segfault arose when iterating over the list of secondary_init_clusters_ and calling initialize(). The call to initialize may result in the initialized item being removed from the secondary_init_clusters_ list while walking through the list. The initialized item can only remove itself, not any other item in the list. However, this resulted in chasing an invalidated iterator when trying to go to the next element in the list. To fix this the loop is modified so that the iterator is advanced to the next item (or end() if no more items) before calling Cluster::initialize(). A test is included which exercises this scenario Fixes envoyproxy#903
mattklein123
reviewed
May 8, 2017
| NiceMock<MockCluster> cluster; | ||
| // Fake that we are in the secondary InitializePhase for this test. | ||
| ON_CALL(cluster, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Secondary)); | ||
| // We expect that initialize() should be called first upon addCluster. In the |
There was a problem hiding this comment.
The code change looks good (thanks for fixing), but I think the test and the explanation can be simplified a bit. For secondary clusters, initialize() is not called when addCluster() is called. It's called once all the primary clusters have initialized OR when static load completes if there are no primary clusters. So, I would rejigger this test to do:
ON_CALL(cluster, initializePhase())...
init_helper.addCluster(cluster);
EXPECT_CALL(cluster, initialize())... // remove inline
init_helper.onStaticLoadComplete();
Then also simplify the explanation surrounding the test.
There was a problem hiding this comment.
OK, thanks! This is definitely useful. I will ensure the rejiggered test fails with the original code, and passes with the fix.
Confirmed that the test still fails with the original maybeFinishInitialize() code, and passes with the revised safe loop.
rshriram
pushed a commit
to rshriram/envoy
that referenced
this pull request
Oct 30, 2018
* Update envoy to 352fe35 * fix format
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A segfault arose when iterating over the list of
secondary_init_clusters_ and calling initialize(). The call to
initialize may result in the initialized item being removed from the
secondary_init_clusters_ list while walking through the list. The
initialized item can only remove itself, not any other item in the
list. However, this resulted in chasing an invalidated iterator when
trying to go to the next element in the list.
To fix this the loop is modified so that the iterator is advanced to the
next item (or end() if no more items) before calling
Cluster::initialize().
A test is included which exercises this scenario
Fixes #903