[libcamera-devel] [PATCH] libcamera: event_notifier_poll: Fix notifier unregistration during event processing

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-07-12 14:04:28 +0300, Laurent Pinchart wrote:
> An event notifier may be unregistered from its activated signal. This
> can cause the notifiers set entry in notifiers_ to be deleted while
> processNotifiers() is looping over the notifiers_ map, leading to
> problems.
> 
> To fix this, add a flag to the EventNotifierPoll class to indicate that
> event processing is in progress. If the flag is set, the notifiers_
> entry is not deleted during notifier unregistration, but will be deleted
> by the event processing loop.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/libcamera/event_dispatcher_poll.cpp       | 18 ++++++++++++++++++
>  src/libcamera/include/event_dispatcher_poll.h |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> index df9dffb2326c..4f15f3e3269b 100644
> --- a/src/libcamera/event_dispatcher_poll.cpp
> +++ b/src/libcamera/event_dispatcher_poll.cpp
> @@ -46,6 +46,7 @@ static const char *notifierType(EventNotifier::Type type)
>   */
>  
>  EventDispatcherPoll::EventDispatcherPoll()
> +	: processingEvents_(false)
>  {
>  	/*
>  	 * Create the event fd. Failures are fatal as we can't implement an
> @@ -96,6 +97,15 @@ void EventDispatcherPoll::unregisterEventNotifier(EventNotifier *notifier)
>  	}
>  
>  	set.notifiers[type] = nullptr;
> +
> +	/*
> +	 * Don't race with event processing if this method is called from an
> +	 * event notifier. The notifiers_ entry will be erased by
> +	 * processEvents().
> +	 */
> +	if (processingEvents_)
> +		return;
> +
>  	if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
>  		notifiers_.erase(iter);
>  }
> @@ -241,6 +251,8 @@ void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pol
>  		{ EventNotifier::Exception, POLLPRI },
>  	};
>  
> +	processingEvents_ = true;
> +
>  	for (const pollfd &pfd : pollfds) {
>  		auto iter = notifiers_.find(pfd.fd);
>  		ASSERT(iter != notifiers_.end());
> @@ -269,7 +281,13 @@ void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pol
>  			if (pfd.revents & event.events)
>  				notifier->activated.emit(notifier);
>  		}
> +
> +		/* Erase the notifiers_ entry if it is now empty. */
> +		if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
> +			notifiers_.erase(iter);
>  	}
> +
> +	processingEvents_ = false;
>  }
>  
>  void EventDispatcherPoll::processTimers()
> diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h
> index 14c3eea13b5e..d82b302c4aea 100644
> --- a/src/libcamera/include/event_dispatcher_poll.h
> +++ b/src/libcamera/include/event_dispatcher_poll.h
> @@ -45,6 +45,8 @@ private:
>  	std::list<Timer *> timers_;
>  	int eventfd_;
>  
> +	bool processingEvents_;
> +
>  	int poll(std::vector<struct pollfd> *pollfds);
>  	void processInterrupt(const struct pollfd &pfd);
>  	void processNotifiers(const std::vector<struct pollfd> &pollfds);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund