Skip to content
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.

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

Fix context impl cleanup. #227

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Fix context impl cleanup. #227

merged 1 commit into from
Aug 28, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 27, 2020

Precisely what the title says. Otherwise, SIGSEGV ensues if a subsequent (re)initialization fails halfway through.

Drop invalid pointers.

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic hidmic requested review from eboasson and ivanpauno August 27, 2020 13:08
@ivanpauno
Copy link
Member

Otherwise, SIGSEGV ensues if a subsequent (re)initialization fails halfway through.

Do we provide that guarantee?

Just to double check, this is what I understand from your comment:

rmw_context_t context = rmw_create_context(...);
rmw_destroy_context(context); // this one fails
rmw_destroy_context(context); // retrying after failing segfaults

@hidmic hidmic changed the title Fix context cleanup. Fix context impl cleanup. Aug 27, 2020
@hidmic
Copy link
Contributor Author

hidmic commented Aug 27, 2020

Just to double check, this is what I understand from your comment:

I meant subsequent context impl (perhaps that's the bit that's missing) (re)initialization failure after a preceding (often implicit) cleanup may result in SIGSEGV. That can happen if, within rmw_create_node, rmw_context_impl_t::init() fails halfway through twice in a row.

@ivanpauno
Copy link
Member

I meant subsequent context impl (perhaps that's the bit that's missing) (re)initialization failure after a preceding (often implicit) cleanup may result in SIGSEGV. That can happen if, within rmw_create_node, rmw_context_impl_t::init() fails halfway through twice in a row.

I see, it's super weird though.

@ivanpauno ivanpauno added the bug Something isn't working label Aug 27, 2020
@hidmic
Copy link
Contributor Author

hidmic commented Aug 27, 2020

CI up to rcl:

@hidmic hidmic merged commit f137db8 into master Aug 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/fix-context-cleanup branch August 28, 2020 17:35
ahcorde pushed a commit that referenced this pull request Oct 30, 2020
Drop invalid pointers.

Signed-off-by: Michel Hidalgo <[email protected]>
ahcorde pushed a commit that referenced this pull request Oct 30, 2020
Drop invalid pointers.

Signed-off-by: Michel Hidalgo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants