search

gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.21k stars 484 forks source link

Racecondition in gazebo::transport::Connection::EnqueueMsg(const std::string &_buffer, boost::function<void(uint32_t)> _cb, uint32_t _id, bool _force) #2387

Open osrf-migration opened 6 years ago

osrf-migration commented 6 years ago

Original report (archived issue) by Hendrik Skubch (Bitbucket: RedundantEntry).

The original report had attachments: gzrace-fix.patch

headerBuffer is subject to a racecondition if multiple writer threads exist (Concurrent calls to EnqueueMsg). This appears to be the case when a model is deleted via a ros-maintained plugin.

Reproduction steps:

  1. Start a gazebo simulation inside a docker container. Add and remove robots continuously via ros plugin and service.
  2. Repeat step 1 until system load is saturated (e.g., 6 simulations on a quad core).
  3. Wait until crash caused by partial messages being parsed and rejected (protobuffer's IsInitialized() fails).

Suggested solution: Apply attached patch. It moves the contest array (9 bytes) from the heap to the stack.

osrf-migration commented 6 years ago

Original comment by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).

Thanks for the patch @RedundantEntry, Would you be willing to create a pull request with the patch targeted at default branch?

osrf-migration commented 6 years ago

Original comment by Hendrik Skubch (Bitbucket: RedundantEntry).

Unfortunately, I don't have the appropriate permissions.

osrf-migration commented 6 years ago

Original comment by Hendrik Skubch (Bitbucket: RedundantEntry).

osrf-migration commented 6 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

@redundantentry if you make a fork of gazebo, you should be have commit permissions:

osrf-migration commented 6 years ago

Original comment by Hendrik Skubch (Bitbucket: RedundantEntry).

Ah, great, thanks for the help! Did that here; https://osrf-migration.github.io/gazebo-gh-pages/#!/osrf/gazebo/pull-requests/2826/move-header-buffer-from-heap-to-stack/diff If you want tests for that, it will take a while longer.

osrf-migration commented 6 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

It's hard to test race conditions; I'll work on testing the patch. Thanks for the submission!