Unsafe Double-checked Locking

[git-afsantos]

I realize this is being compiled with C++03, and, thus, the workarounds are not possible, as they require C++11. Here is the faulty code, in roscpp/src/libros/topic_manager.cpp (line 56).

TopicManagerPtr g_topic_manager;
boost::mutex g_topic_manager_mutex;
const TopicManagerPtr& TopicManager::instance()
{
  if (!g_topic_manager)
  {
    boost::mutex::scoped_lock lock(g_topic_manager_mutex);
    if (!g_topic_manager)
    {
      g_topic_manager = boost::make_shared<TopicManager>();
    }
  }

  return g_topic_manager;
}

Double-checked locking as is done here is potentially unsafe; it is possible that a thread may read g_topic_manager before it is fully initialized. Of course, it depends on whether there are multiple threads contesting the access (which I am not sure about).

Is lazy initialization really necessary here?

[dirk-thomas]

Can you please describe in more detail why you think this code is unsafe.

To me it looks like that the outer condition is checked atomically. And if it is not yet initialized the inner check and creation of the singleton is properly protected by the mutex.

[git-afsantos]

I have little experience in C++, however, from my brief research, it is vulnerable in the same way Java code is. Consider the assignment statement.

g_topic_manager = boost::make_shared<TopicManager>();

There are three things happening here:

1. Allocation of memory;
2. Construction of the object on the allocated memory;
3. Pointing to the allocated memory.

Compilers are not forced to follow this ordering. This means that step 3 can occur before step 2, and here lies the problem.

• Thread A goes through steps 1 and 3 and is scheduled out;
• Thread B enters the method, reads g_topic_manager (which is now not null) and returns;
• Thread B is able to use g_topic_manager before Thread A goes through step 2.

If this does not make much sense, I will leave you with my references:

• I have been reading on the issue here;
• I have opened a question on StackOverflow to ask exactly this, and the answers confirm that it is unsafe.

This is an issue only if multiple threads try to access the topic manager for the first time, at the same time. I am not familiar enough with the source code to know if this is possible (but I imagine it is, hence the mutex in the first place).

[dirk-thomas]

Very interesting. Thanks for the detailed links. I will change the label to bug since the current code is certainly problematic.

In the upcoming ROS Kinetic release we require a C++11 compiler. Probably the best solution is to change the code in a to be made kinetic-devel branch to use the local static variable.

[git-afsantos]

Thanks, I am glad I could help.
If the next release is based on C++11, then the static variable seems to be the way to go, indeed. 👍

[dirk-thomas]

Please see #776 for the proposed change in Kinetic.