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 38fa0c6fcfde235219644e670599af2b98c1b3f3..c2887e4737ae6dc8123d5ea6da9099de85d6c0bc 100644 (file)
--- 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 31ecb79765e2928125aa428f9d075d30d885135f..f2e54030ef9caef364408cf956e67a46ce71d888 100644 (file)
--- 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 b0bcb5d62eb27f03d304d548d7e908d5247c2a9f..e799aa7595a87936680585362637e5a9bec26660 100644 (file)
--- 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 f7b6ee28364399986d39f9df5b5337c44ecb04e0..60244512400370e22accc758bebc40e52a77d32a 100644 (file)
--- 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());