Fix dangerous std::bind capture in TimeSource implementation

[gbiggs]

The implementation of the TimeSource object, in time_source.cpp, has two instances of a std::bind capture that is dangerous:

1. On line 132: std::bind(&TimeSource::on_parameter_event, this, std::placeholders::_1));
2. On line 280: std::bind(&TimeSource::clock_cb, this, std::placeholders::_1),

These captures are dangerous because they capture this as a raw pointer. This does not protect the lifetime of the pointer, potentially leading to a race condition if the bound function is called after the class instance is destructed in any way.

The first instance above leads to a race condition and a segmentation fault in one of the tests in OpenRMF. In short, what happens is that the test ends and begins shutting down the ROS infrastructure. This shuts down the node, which detaches it from the TimeSource. However, just after that happens a context switch occurs and a parameter event begins being handled by the on_parameter_event function of TimeSource - the destination of that first std::bind call, above. The first line of that function tries to use the node_base_ pointer which has just been reset by the other thread in detach_node(). The result is a segmentation fault.

The fix is to shift the TimeSource object's internal state into a private structure that can be managed with a shared pointer, and passing a weak pointer to a lambda that calls the callback. The lambda first locks the weak pointer. If it cannot lock the weak pointer then it knows the state has gone and so the callback should not be executed. If it does lock the weak pointer, then even resetting it in another thread won't result in immediate deletion of the state. This allows the life time of the internal state to be extended to cover the callback when necessary, removing the race condition.

Running the test in question fails with a segmentation fault about 10% of the time. Fixing the bad capture eliminates the segmentation fault. Along with fixing the bad capture that caused the segmentation fault, this PR also fixes the other bad capture in TimeSource.

This PR splits the internal state of TimeSource into two separate objects that can be managed individually. This is done to allow the mostly-independent states to have their life times managed independently, maintaining the ability to attach and detach nodes independently of attaching and detaching clocks.

[gbiggs]

For additional context on why the race condition occurs, see this presentation from @mxgrey. Specifically the slides from this point describe how to use lambda and bind captures safely.

Note that a lambda of the form [this]() { ... } is equivalent to std::bind(..., this, ...). This is the pattern found in TimeSource.

[clalancette]

@gbiggs This is failing the linters, can you take a look?

[ivanpauno]

I have to take a deeper look, but the approach taken makes sense to me.

I have left some style related comments.

[ivanpauno]

AFAIS this can cause an use-after-free, in the same way as before.

NodeState weak ptr in one of the callbacks is locked, while node_state_ member of time source was still valid.
Then, a context switch happens and the time source gets destroyed.
Now, the callback has ownership of a NodeState ptr pointing to a dangling clocks_state_.

IMO, though the conceptual distintion between "ClocksState" and "NodeState" is nice, it's making the things more complex.
You can have only one TimeSourceImpl pointer, and if you want to keep the conceptual difference you use:

struct TimeSourceImpl {
  ClocksState clocks_state;
  NodeState node_state;
};

In that case NodeState can still keep a reference to ClocksState, but maybe it's better to explicitly pass it as an argument where needed.

This is not the only solution, e.g. you could simple update this line too std::shared_ptr<ClocksState>, but I don't see much reason to have more than one pointer to implementation in the same class.

[gbiggs]

I switched the NodeState's reference to the ClockState to be a weak pointer in 74c28d0. This allows it to be checked for validity and prevent it going out of scope in the middle of a callback.

The distinction between the ClockState and the NodeState is not conceptual. Ideally I'd prefer not to have it. The reason it's there is because the TimeSource API as currently written and implemented allows clocks to be added and removed independently of attaching and detaching a node. This extends to being able to add a clock when there is no attached node. The cleanest way to maintain that is to have two separate states with independent life times, so whenever there is an attached node the NodeState instance has meaning, and whenever there is one or more clocks attached the ClockState has meaning.

I don't know if the TimeSource class is actually used in a way where it needs to have clocks without an attached node, or vice versa. I haven't checked every use of TimeSource. My goal was to have no changes in existing API and behaviour. We need this fix to be backported to Galactic (even though ABI is almost certainly broken by it) because it is a guaranteed segmentation fault in Galactic in our CI.

[ivanpauno]

The distinction between the ClockState and the NodeState is not conceptual. Ideally I'd prefer not to have it. The reason it's there is because the TimeSource API as currently written and implemented allows clocks to be added and removed independently of attaching and detaching a node. This extends to being able to add a clock when there is no attached node. The cleanest way to maintain that is to have two separate states with independent life times, so whenever there is an attached node the NodeState instance has meaning, and whenever there is one or more clocks attached the ClockState has meaning.

Sorry, I don't understand the reasoning.
AFAIS, the problem could've been solved by moving the current TimeSource implementation altogether to a TimeSourceImpl class, enabling shared_from_this in that class and passing a weak ptr to the callbacks.
I'm not sure why two independent states with different lifetimes are needed.

[mxgrey]

It's a very subtle issue, but the gist of it is that the lifecycles of "the fields that are related to nodes" (which we're putting into node_state_) needs to be independent from the lifecycles of "the fields related to clocks" (which we're putting into clocks_state_).

To prevent use-after-free issues, we capture a std::weak_ptr<NodeState> inside a lambda that handles parameter callbacks. Before that lambda can modify the NodeState, it needs to lock the weak_ptr. Locking the weak_ptr ensures this specific NodeState instance remains alive while the lambda operates on it. That much is obvious, but here's where it gets tricky:

To avoid lifecycle race conditions that may occur when TimeSource::detachNode() is called at the same time as the parameter callback lambda happens to be running, we have tweaked the implementation of TimeSource::detachNode() to simply release its hold on the entire NodeState instance and create a whole newly allocated NodeState instance. Therefore, when TimeSource::detachNode() is called at the same time as the parameter callback lambda, the original NodeState instance will stay alive right up until the lambda is finished executing. Once the lambda is finished executing, the ~NodeState() destructor will be called for the original NodeState instance, freeing its resources and unregistering its callbacks.

If we grouped all the clock-related fields into the same shared struct as the node-related fields, then we would have to kill and remake all the clock-related fields each time we detach a node. This might technically be doable, but I think this would actually add complexity to the overall implementation, because we need to be conscientious about copying certain fields from the original struct and not others when we detach a node.

Instead of that, we are separating out the clock-related fields which are independent from the node-related fields, which I believe gives us a better "separation of concerns".

If we want to fully "pimpl-fy" the TimeSource class, we could certainly create a TimeSourceImpl class, but then (imo) the implementation of that class should still separate out the node fields from the clock fields into different class instances whose lifecycles can be managed independently from each other.

[ivanpauno]

I see, the issue is tricky.

It seems that all the attachNode/detachNode/attachClock/detachClock are a bit unnecessary.
The only time we're using the attach*() methods is right after construction, and we never call the detach*() ones (except implicitly, from the destructor).
I would rather see in the future all of them deleted, it will simplify the code quite a lot.
For the moment, this fix seems correct.

[mxgrey]

I agree that we should simply remove them if they're not actually being used and there is no intention of supporting them in the public API. Perhaps as a safety step, we can mark them as deprecated and link to this PR in the function documentation.

[ivanpauno]

I agree that we should simply remove them if they're not actually being used and there is no intention of supporting them in the public API. Perhaps as a safety step, we can mark them as deprecated and link to this PR in the function documentation.

That sounds like a good idea to me, though we can do that in a different PR.
@wjwwood what do you think?

[ivanpauno]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

@gbiggs I'm not sure, but it looks like this needs to be rebased before running CI.

[clalancette]

Yeah, it definitely does. It needs the changes from #1788 to compile.

[gbiggs]

@ivanpauno I've rebased it. Can you please run CI again?

[ivanpauno]

sorry for the delay, I was OoO.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[mxgrey]

Are the unstable statuses anything to be worried about?

[clalancette]

Are the unstable statuses anything to be worried about?

Both of those failures were on the nightlies up until a few days ago:

• https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_release/1712/#showFailuresLink
• https://ci.ros2.org/view/nightly/job/nightly_osx_release/2215/#showFailuresLink

However, they are no longer there from last night:

• https://ci.ros2.org/view/nightly/job/nightly_osx_release/2223/
• https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_release/1720/

That suggests that they were fixed elsewhere. Nevertheless, in order to make sure this PR doesn't re-introduce the problem, I'll suggest rebasing one more time and running CI one more time.

[gbiggs]

I've rebased.

[clalancette]

Thanks, one more CI:

• Linux
• Linux-aarch64
• macOS
• Windows