[libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls to dispatchMessages()

[David Plowman]

Hi Laurent

Thank you very much for this patch.

On Fri, 2 Jul 2021 at 00:08, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> There are use cases for calling the dispatchMessages() function
> recursively, from within a message handler. This can be used, for
> instance, to force delivery of messages posted to a thread concurrently
> to stopping the thread. This currently causes access, in the outer
> dispatchMessages() call, to iterators that have been invalidated by
> erasing list elements in the recursive call, leading to undefined
> behaviour (most likely double-free or other crashes).
>
> Fix it by only erasing messages from the list at the end of the outer
> call, identified using a recursion counter. We need to keep track of the
> first non-null entry in the posted messages list to restrict the
> deletion to the
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=26
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Notwithstanding the subsequent discussion, I've been running with
these changes for a while now and haven't seen the problem return, so:

Tested-by: David Plowman <david.plowman at raspberrypi.com>

Thank you!
David

> ---
>  src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 7f79115222e8..9eb488d46db8 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -126,6 +126,11 @@ public:
>          * \brief Protects the \ref list_
>          */
>         Mutex mutex_;
> +       /**
> +        * \brief The recursion level for recursive Thread::dispatchMessages()
> +        * calls
> +        */
> +       unsigned int recursion_ = 0;
>  };
>
>  /**
> @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)
>   * Messages shall only be dispatched from the current thread, typically within
>   * the thread from the run() function. Calling this function outside of the
>   * thread results in undefined behaviour.
> + *
> + * This function is not thread-safe, but it may be called recursively in the
> + * same thread from an object's message handler. It guarantees in all cases
> + * delivery of messages in the order they have been posted.
>   */
>  void Thread::dispatchMessages(Message::Type type)
>  {
>         ASSERT(data_ == ThreadData::current());
>
> +       ++data_->messages_.recursion_;
> +
>         MutexLocker locker(data_->messages_.mutex_);
>
>         std::list<std::unique_ptr<Message>> &messages = data_->messages_.list_;
>
> -       for (auto iter = messages.begin(); iter != messages.end(); ) {
> -               std::unique_ptr<Message> &msg = *iter;
> -
> -               if (!msg) {
> -                       iter = data_->messages_.list_.erase(iter);
> +       for (std::unique_ptr<Message> &msg : messages) {
> +               if (!msg)
>                         continue;
> -               }
>
> -               if (type != Message::Type::None && msg->type() != type) {
> -                       ++iter;
> +               if (type != Message::Type::None && msg->type() != type)
>                         continue;
> -               }
>
> +               /*
> +                * Move the message, setting the entry in the list to null. It
> +                * will cause recursive calls to ignore the entry, and the erase
> +                * loop at the end of the function to delete it from the list.
> +                */
>                 std::unique_ptr<Message> message = std::move(msg);
> -               iter = data_->messages_.list_.erase(iter);
>
>                 Object *receiver = message->receiver_;
>                 ASSERT(data_ == receiver->thread()->data_);
> @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)
>                 message.reset();
>                 locker.lock();
>         }
> +
> +       /*
> +        * If the recursion level is 0, erase all null messages in the list. We
> +        * can't do so during recursion, as it would invalidate the iterator of
> +        * the outer calls.
> +        */
> +       if (!--data_->messages_.recursion_) {
> +               for (auto iter = messages.begin(); iter != messages.end(); ) {
> +                       if (!*iter)
> +                               iter = messages.erase(iter);
> +                       else
> +                               ++iter;
> +               }
> +       }
>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>