| Message ID | 20190712110428.20433-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 3e14a1bcb0557e73718d12cd7ef113940472a934 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for the patch. On Fri, Jul 12, 2019 at 02:04:28PM +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@ideasonboard.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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);
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@ideasonboard.com> --- src/libcamera/event_dispatcher_poll.cpp | 18 ++++++++++++++++++ src/libcamera/include/event_dispatcher_poll.h | 2 ++ 2 files changed, 20 insertions(+)