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 segfault caused by destruction order of Event and Connection #234

Merged

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jul 28, 2021

🦟 Bug fix

Fixes gazebosim/gz-sim#906

Summary

A segfault occurs in the destructor of common::Connection if the common::Event its associated with has already been deleted. This patch clears the event pointer of common::Connection when the associated common::Event is deleted.

Another issue mentioned in gazebosim/gz-sim#906 is that the destructor of std::function crashes if the function it points to is from a shared library that has been unloaded. This is fixed by invoking the destructor as soon as common:EventT::Disconnect is called via common::Connection::~Connection since that's likely to be right before the shared library containing the common::Connection is unloaded.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@azeey azeey requested a review from mjcarroll July 28, 2021 00:21
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Jul 28, 2021
@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #234 (d4f7867) into ign-common3 (c6bddd8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3     #234      +/-   ##
===============================================
+ Coverage        76.53%   76.55%   +0.01%     
===============================================
  Files               73       73              
  Lines            10379    10386       +7     
===============================================
+ Hits              7944     7951       +7     
  Misses            2435     2435              
Impacted Files Coverage Δ
events/include/ignition/common/Event.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6bddd8...d4f7867. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants