From: Amos Jeffries <squid3@treenet.co.nz>
Date: Sat, 13 Aug 2016 10:12:06 +0000 (+1200)
Subject: Bug 4561: Replace use of default move operators with explicit implementation
X-Git-Tag: SQUID_4_0_14~34
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b9a9207b64526dc5f0a137896b01d578806aee2f;p=thirdparty%2Fsquid.git

Bug 4561: Replace use of default move operators with explicit implementation

The '= default' syntax for move assignment operator and constructor can
result in double-free crashes when the class type containspointer members
since the trivial move uses std::memmove() which does not perform the
required zeroing/nullptr of the temporary source object being moved.
---

diff --git a/src/base/CbcPointer.h b/src/base/CbcPointer.h
index 38fa0c6fcf..c2887e4737 100644
--- a/src/base/CbcPointer.h
+++ b/src/base/CbcPointer.h
@@ -28,6 +28,7 @@ public:
     CbcPointer(); // a nil pointer
     CbcPointer(Cbc *aCbc);
     CbcPointer(const CbcPointer &p);
+    CbcPointer(CbcPointer &&);
     ~CbcPointer();
 
     Cbc *raw() const; ///< a temporary raw Cbc pointer; may be invalid
@@ -42,6 +43,7 @@ public:
     bool operator ==(const CbcPointer<Cbc> &o) const { return lock == o.lock; }
 
     CbcPointer &operator =(const CbcPointer &p);
+    CbcPointer &operator =(CbcPointer &&);
 
     /// support converting a child cbc pointer into a parent cbc pointer
     template <typename Other>
@@ -99,6 +101,13 @@ CbcPointer<Cbc>::CbcPointer(const CbcPointer &d): cbc(d.cbc), lock(NULL)
         lock = cbdataReference(d.lock);
 }
 
+template<class Cbc>
+CbcPointer<Cbc>::CbcPointer(CbcPointer &&d): cbc(d.cbc), lock(d.lock)
+{
+    d.cbc = nullptr;
+    d.lock = nullptr;
+}
+
 template<class Cbc>
 CbcPointer<Cbc>::~CbcPointer()
 {
@@ -117,6 +126,19 @@ CbcPointer<Cbc> &CbcPointer<Cbc>::operator =(const CbcPointer &d)
     return *this;
 }
 
+template<class Cbc>
+CbcPointer<Cbc> &CbcPointer<Cbc>::operator =(CbcPointer &&d)
+{
+    if (this != &d) { // assignment to self
+        clear();
+        cbc = d.cbc;
+        d.cbc = nullptr;
+        lock = d.lock;
+        d.lock = nullptr;
+    }
+    return *this;
+}
+
 template<class Cbc>
 void
 CbcPointer<Cbc>::clear()
diff --git a/src/base/RegexPattern.cc b/src/base/RegexPattern.cc
index 31ecb79765..f2e54030ef 100644
--- a/src/base/RegexPattern.cc
+++ b/src/base/RegexPattern.cc
@@ -16,9 +16,28 @@ RegexPattern::RegexPattern(int aFlags, const char *aPattern) :
     memset(&regex, 0, sizeof(regex));
 }
 
+RegexPattern::RegexPattern(RegexPattern &&o) :
+    flags(std::move(o.flags)), // does o.flags=0
+    regex(std::move(o.regex)),
+    pattern(std::move(o.pattern))
+{
+    memset(&o.regex, 0, sizeof(o.regex));
+    o.pattern = nullptr;
+}
+
 RegexPattern::~RegexPattern()
 {
     xfree(pattern);
     regfree(&regex);
 }
 
+RegexPattern &
+RegexPattern::operator =(RegexPattern &&o)
+{
+    flags = std::move(o.flags); // does o.flags=0
+    regex = std::move(o.regex);
+    memset(&o.regex, 0, sizeof(o.regex));
+    pattern = std::move(o.pattern);
+    o.pattern = nullptr;
+    return *this;
+}
diff --git a/src/base/RegexPattern.h b/src/base/RegexPattern.h
index b0bcb5d62e..e799aa7595 100644
--- a/src/base/RegexPattern.h
+++ b/src/base/RegexPattern.h
@@ -23,10 +23,15 @@ class RegexPattern
 public:
     RegexPattern() = delete;
     RegexPattern(int aFlags, const char *aPattern);
-    RegexPattern(const RegexPattern &) = delete;
-    RegexPattern(RegexPattern &&) = default;
     ~RegexPattern();
 
+    // regex type varies by library, usually not safe to copy
+    RegexPattern(const RegexPattern &) = delete;
+    RegexPattern &operator =(const RegexPattern &) = delete;
+
+    RegexPattern(RegexPattern &&);
+    RegexPattern &operator =(RegexPattern &&);
+
     const char * c_str() const {return pattern;}
     bool match(const char *str) const {return regexec(&regex,str,0,NULL,0)==0;}
 
diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h
index f7b6ee2836..6024451240 100644
--- a/src/security/LockingPointer.h
+++ b/src/security/LockingPointer.h
@@ -74,8 +74,9 @@ public:
         return *this;
     }
 
-    // move semantics are definitely okay, when possible
-    explicit LockingPointer(SelfType &&) = default;
+    LockingPointer(SelfType &&o) : raw(nullptr) {
+        resetWithoutLocking(o.release());
+    }
     SelfType &operator =(SelfType &&o) {
         if (o.get() != raw)
             resetWithoutLocking(o.release());