From 6e06d1db6045df2e4961243f379c4d7695a8190d Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 7 Oct 2021 15:32:19 -0400
Subject: SL-16024: Make LLCond::get() lock and return by value.

Its previous behavior, returning a const reference without locking, was wrong:
it could return a reference to an object in an inconsistent state if it was
concurrently being modified on another thread.

Locking the mutex and returning a copy by value is the correct behavior.
---
 indra/llcommon/llcond.h | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

(limited to 'indra')

diff --git a/indra/llcommon/llcond.h b/indra/llcommon/llcond.h
index e31b67d893..c08acb66a1 100644
--- a/indra/llcommon/llcond.h
+++ b/indra/llcommon/llcond.h
@@ -53,6 +53,8 @@ private:
     LLCoros::Mutex mMutex;
     // Use LLCoros::ConditionVariable for the same reason.
     LLCoros::ConditionVariable mCond;
+    using LockType = LLCoros::LockType;
+    using cv_status = LLCoros::cv_status;
 
 public:
     /// LLCond can be explicitly initialized with a specific value for mData if
@@ -65,10 +67,14 @@ public:
     LLCond(const LLCond&) = delete;
     LLCond& operator=(const LLCond&) = delete;
 
-    /// get() returns a const reference to the stored DATA. The only way to
-    /// get a non-const reference -- to modify the stored DATA -- is via
-    /// update_one() or update_all().
-    const value_type& get() const { return mData; }
+    /// get() returns the stored DATA by value -- so to use get(), DATA must
+    /// be copyable. The only way to get a non-const reference -- to modify
+    /// the stored DATA -- is via update_one() or update_all().
+    value_type get()
+    {
+        LockType lk(mMutex);
+        return mData;
+    }
 
     /**
      * Pass update_one() an invocable accepting non-const (DATA&). The
@@ -83,7 +89,7 @@ public:
     void update_one(MODIFY modify)
     {
         { // scope of lock can/should end before notify_one()
-            LLCoros::LockType lk(mMutex);
+            LockType lk(mMutex);
             modify(mData);
         }
         mCond.notify_one();
@@ -102,7 +108,7 @@ public:
     void update_all(MODIFY modify)
     {
         { // scope of lock can/should end before notify_all()
-            LLCoros::LockType lk(mMutex);
+            LockType lk(mMutex);
             modify(mData);
         }
         mCond.notify_all();
@@ -118,7 +124,7 @@ public:
     template <typename Pred>
     void wait(Pred pred)
     {
-        LLCoros::LockType lk(mMutex);
+        LockType lk(mMutex);
         // We must iterate explicitly since the predicate accepted by
         // condition_variable::wait() requires a different signature:
         // condition_variable::wait() calls its predicate with no arguments.
@@ -205,14 +211,14 @@ private:
     template <typename Clock, typename Duration, typename Pred>
     bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time, Pred pred)
     {
-        LLCoros::LockType lk(mMutex);
+        LockType lk(mMutex);
         // We advise the caller to pass a predicate accepting (const DATA&).
         // But what if they instead pass a predicate accepting non-const
         // (DATA&)? Such a predicate could modify mData, which would be Bad.
         // Forbid that.
         while (! pred(const_cast<const value_type&>(mData)))
         {
-            if (LLCoros::cv_status::timeout == mCond.wait_until(lk, timeout_time))
+            if (cv_status::timeout == mCond.wait_until(lk, timeout_time))
             {
                 // It's possible that wait_until() timed out AND the predicate
                 // became true more or less simultaneously. Even though
-- 
cgit v1.2.3