search

ros-drivers / libuvc_ros

http://www.ros.org/wiki/libuvc_ros
82 stars 95 forks source link

Fixed locks so they stay in scope until end of method. #36

Closed jasonimercer closed 5 years ago

7675t commented 7 years ago

Hi, I'm not familiar with this APIs, what bad things happen without this PR?

jasonimercer commented 7 years ago

It looks like the original author was trying to protect the scope of these methods from multi-threaded access but wrote the lock incorrectly. The current code will block on the locking call until the mutex is free, lock on the mutex and then, since the variable is not named, it will go out of scope and unlock the mutex. The proposed code will wait on the mutex, acquire and hold the lock until the end of the method when the named variable goes out of scope and standard RAII will release the lock. This will protect the variables and calls inside the method from multi-threaded access.

The proposed fix is not tested and with locks you run the risk of a deadlock if used incorrectly. Without proper locks multi-threaded access can lean to data corruption or crashes.

I didn't find this bug by observing anything bad happen in this repo, I did a code search on all packages my company uses or forked and found a few that have this pattern. I think the original search was motivated by an internal bug where we used a lock incorrectly - I can't really remember as it's been almost a year since I posted this fix.