Bounds-check EventHandlerBase::is_ready() to fix out-of-bounds read (#2376)#3170
Open
PavelGuzenfeld wants to merge 1 commit into
Open
Conversation
…2376) is_ready() indexed wait_set.events with wait_set_event_index_, which is only assigned once the handler is added to that wait set via add_to_wait_set(). If is_ready() runs first (e.g. a publisher/subscription event thread racing node setup), the index is uninitialized or refers to a different wait set, so the access reads out of bounds and can segfault (observed with ASan during lifecycle configure). Initialize wait_set_event_index_ and make is_ready() return false when the index is not within the wait set's events instead of dereferencing it. Initializing alone is not sufficient (index 0 may still not be this handler's slot), so the bounds check is the actual guard. Adds a regression test that calls is_ready() on a handler that was never added to a wait set. Refs ros2#2376 Co-authored-by: Mike Wake <michael.wake@aosgrp.com.au> Generated-by: Claude Opus 4.8 (Anthropic) Signed-off-by: Pavel Guzenfeld <pavelguzenfeld@gmail.com> Signed-off-by: Mike Wake <michael.wake@aosgrp.com.au>
PavelGuzenfeld
force-pushed
the
fix/issue-2376-event-handler-is-ready-bounds
branch
from
7aa7462 to
048bc4d
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
EventHandlerBase::is_ready()indexeswait_set.events[wait_set_event_index_], but that index is only assigned once the handler is added to the wait set viaadd_to_wait_set(). Ifis_ready()runs first — e.g. a publisher/subscription event thread racing node setup — the index is not valid for this wait set and the access reads out of bounds, which can segfault (observed with ASan during a lifecycleconfigure).This initializes
wait_set_event_index_to an always-out-of-range sentinel and makesis_ready()return false when the index is not within the wait set's events, instead of dereferencing it.Adds a regression test that calls
is_ready()on a handler that was never added to a wait set.Builds on @aosmw's work in #2376; per @clalancette's review there, initializing the index alone is not sufficient (it could still point at another slot), so the bounds check in
is_ready()is the actual guard.Some of this change was produced with Claude Opus 4.8 (Anthropic).