[libcamera-devel] libcamera: base: thread: Protect exitCode by mutex in exit()
diff mbox series

Message ID 20220620051150.361575-1-umang.jain@ideasonboard.com
State Not Applicable
Delegated to: Umang Jain
Headers show
Series
  • [libcamera-devel] libcamera: base: thread: Protect exitCode by mutex in exit()
Related show

Commit Message

Umang Jain June 20, 2022, 5:11 a.m. UTC 
From: Hirokazu Honda <hiroh@chromium.org>

Thread::exit() accesses data_->exitCode without acquiring a lock.
Fix it.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/libcamera/base/thread.cpp | 3 +++
 1 file changed, 3 insertions(+)

Comments

Laurent Pinchart June 20, 2022, 8:27 a.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Mon, Jun 20, 2022 at 10:41:50AM +0530, Umang Jain via libcamera-devel wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
> 
> Thread::exit() accesses data_->exitCode without acquiring a lock.
> Fix it.

Could you explain here why the lock is needed ?

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/libcamera/base/thread.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 6bda9d14..ff65641e 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -392,7 +392,10 @@ void Thread::finishThread()
>   */
>  void Thread::exit(int code)
>  {
> +	data_->mutex_.lock();
>  	data_->exitCode_ = code;
> +	data_->mutex_.unlock();
> +
>  	data_->exit_.store(true, std::memory_order_release);
>  
>  	EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_relaxed);
Umang Jain June 20, 2022, 8:47 a.m. UTC | #2 
Hi,

On 6/20/22 13:57, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Jun 20, 2022 at 10:41:50AM +0530, Umang Jain via libcamera-devel wrote:
>> From: Hirokazu Honda <hiroh@chromium.org>
>>
>> Thread::exit() accesses data_->exitCode without acquiring a lock.
>> Fix it.
> Could you explain here why the lock is needed ?

This is originally from "Apply clang thread safety annotation 
libcamera-core and v4l2" series

Isn't exitCode_ should always be accessed with the lock? It seems so, in 
the Thread class.

I get it it's exit() / cleanup part so we might not care of the locking,
but when we introduce thread annotation, it becomes a source of 
complaining, hence the patch.

>
>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/libcamera/base/thread.cpp | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
>> index 6bda9d14..ff65641e 100644
>> --- a/src/libcamera/base/thread.cpp
>> +++ b/src/libcamera/base/thread.cpp
>> @@ -392,7 +392,10 @@ void Thread::finishThread()
>>    */
>>   void Thread::exit(int code)
>>   {
>> +	data_->mutex_.lock();
>>   	data_->exitCode_ = code;
>> +	data_->mutex_.unlock();
>> +
>>   	data_->exit_.store(true, std::memory_order_release);
>>   
>>   	EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_relaxed);

Patch
diff mbox series 

diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
index 6bda9d14..ff65641e 100644
--- a/src/libcamera/base/thread.cpp
+++ b/src/libcamera/base/thread.cpp
@@ -392,7 +392,10 @@  void Thread::finishThread()
  */
 void Thread::exit(int code)
 {
+	data_->mutex_.lock();
 	data_->exitCode_ = code;
+	data_->mutex_.unlock();
+
 	data_->exit_.store(true, std::memory_order_release);
 
 	EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_relaxed);