From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 21 Feb 2024 21:37:27 +0000 (-0500)
Subject: cpp-common/bt2: use `std::unique_ptr` to manage `UserMessageIterator::_mSavedLibError`
X-Git-Url: https://git.efficios.com/?p=babeltrace.git;a=commitdiff_plain;h=ea0030320bb23805e0f7a340930e165e84e8eaf0

cpp-common/bt2: use `std::unique_ptr` to manage `UserMessageIterator::_mSavedLibError`

clang-tidy points out that `UserMessageIterator` has a user-defined
destructor, but default copy constructors and assignment operators.
This is indeed dangerous: if we have an error in `_mSavedLibError` and
the `UserMessageIterator` object gets copied, the error will be released
multiple times, causing a use-after-free.

The most obvious fix would be to explicitly delete the default
definitions of the copy constructors and assignment operators.  But
I think it's a bit more idiomatic C++ to use some RAII type to manage
the error.  In this case `std::unique_ptr` is a good fit.  This lets us
delete the user-defined destructor.  The copy constructors and
assignment operators are implicitly deleted, since `unique_ptr` doesn't
have them.

Change-Id: Ia78bbda6a342192cb35c1b6963d3ad7fccaa7c3f
Signed-off-by: Simon Marchi <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/11844
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>
---

diff --git a/src/cpp-common/bt2/component-class-dev.hpp b/src/cpp-common/bt2/component-class-dev.hpp
index 9e2969ec..f9722836 100644
--- a/src/cpp-common/bt2/component-class-dev.hpp
+++ b/src/cpp-common/bt2/component-class-dev.hpp
@@ -487,18 +487,13 @@ protected:
     }
 
 public:
-    ~UserMessageIterator()
-    {
-        this->_resetError();
-    }
-
     void next(bt2::ConstMessageArray& messages)
     {
         /* Any saved error? Now is the time to throw */
         if (G_UNLIKELY(_mExcToThrowType != _ExcToThrowType::NONE)) {
             /* Move `_mSavedLibError`, if any, as current thread error */
             if (_mSavedLibError) {
-                BT_CURRENT_THREAD_MOVE_ERROR_AND_RESET(_mSavedLibError);
+                bt_current_thread_move_error(_mSavedLibError.release());
             }
 
             /* Throw the corresponding exception */
@@ -549,7 +544,7 @@ public:
                 "An error occurred, but there are {} messages to return: delaying the error reporting.",
                 messages.length());
             BT_ASSERT(!_mSavedLibError);
-            _mSavedLibError = bt_current_thread_take_error();
+            _mSavedLibError.reset(bt_current_thread_take_error());
         }
     }
 
@@ -629,10 +624,7 @@ private:
     void _resetError() noexcept
     {
         _mExcToThrowType = _ExcToThrowType::NONE;
-
-        if (_mSavedLibError) {
-            bt_error_release(_mSavedLibError);
-        }
+        _mSavedLibError.reset();
     }
 
     SelfMessageIterator _mSelfMsgIter;
@@ -646,7 +638,16 @@ private:
      * It also saves the type of the exception to throw the next time.
      */
     _ExcToThrowType _mExcToThrowType = _ExcToThrowType::NONE;
-    const bt_error *_mSavedLibError = nullptr;
+
+    struct LibErrorDeleter final
+    {
+        void operator()(const bt_error * const error) const noexcept
+        {
+            bt_error_release(error);
+        }
+    };
+
+    std::unique_ptr<const bt_error, LibErrorDeleter> _mSavedLibError;
 
 protected:
     bt2c::Logger _mLogger;