Browse Source

Fix crash when unmounting/closing multifile while streams are open

It's not really reasonable to expect a user to find every occurrence of a cached resource that might be using an open stream and remove it or crash otherwise.

This is fixed by keeping the multifile stream open as long as any substreams are still pointing to it, using a simplified reference counting (care is taken not to fully make StreamWrapper reference-counted, since it's not in express and existing uses should not be broken).

Fixes #449
Also see #428
rdb 7 years ago
parent
commit
32df05b528

+ 18 - 0
dtool/src/prc/streamWrapper.I

@@ -58,6 +58,24 @@ release() {
   _lock.unlock();
 }
 
+/**
+ * Increments the reference count.  Only has impact if the class that manages
+ * this StreamWrapper's lifetime (eg. Multifile) respects it.
+ */
+INLINE void StreamWrapperBase::
+ref() const {
+  AtomicAdjust::inc(_ref_count);
+}
+
+/**
+ * Decrements the reference count.  Only has impact if the class that manages
+ * this StreamWrapper's lifetime (eg. Multifile) respects it.
+ */
+INLINE bool StreamWrapperBase::
+unref() const {
+  return AtomicAdjust::dec(_ref_count);
+}
+
 /**
  *
  */

+ 10 - 0
dtool/src/prc/streamWrapper.h

@@ -16,6 +16,7 @@
 
 #include "dtoolbase.h"
 #include "mutexImpl.h"
+#include "atomicAdjust.h"
 
 /**
  * The base class for both IStreamWrapper and OStreamWrapper, this provides
@@ -30,8 +31,17 @@ PUBLISHED:
   INLINE void acquire();
   INLINE void release();
 
+public:
+  INLINE void ref() const;
+  INLINE bool unref() const;
+
 private:
   MutexImpl _lock;
+
+  // This isn't really designed as a reference counted class, but it is useful
+  // to treat it as one when dealing with substreams created by Multifile.
+  mutable AtomicAdjust::Integer _ref_count = 1;
+
 #ifdef SIMPLE_THREADS
   // In the SIMPLE_THREADS case, we need to use a bool flag, because MutexImpl
   // defines to nothing in this case--but we still need to achieve a form of

+ 4 - 1
panda/src/express/multifile.cxx

@@ -333,7 +333,10 @@ close() {
   if (_owns_stream) {
     // We prefer to delete the IStreamWrapper over the ostream, if possible.
     if (_read != nullptr) {
-      delete _read;
+      // Only delete it if no SubStream is still referencing it.
+      if (!_read->unref()) {
+        delete _read;
+      }
     } else if (_write != nullptr) {
       delete _write;
     }

+ 13 - 0
panda/src/express/subStreamBuf.cxx

@@ -88,6 +88,13 @@ open(IStreamWrapper *source, OStreamWrapper *dest, streampos start, streampos en
   _append = append;
   _gpos = _start;
   _ppos = _start;
+
+  if (source != nullptr) {
+    source->ref();
+  }
+  if (dest != nullptr) {
+    dest->ref();
+  }
 }
 
 /**
@@ -98,6 +105,12 @@ close() {
   // Make sure the write buffer is flushed.
   sync();
 
+  if (_source != nullptr && !_source->unref()) {
+    delete _source;
+  }
+  if (_dest != nullptr && !_dest->unref()) {
+    delete _dest;
+  }
   _source = nullptr;
   _dest = nullptr;
   _start = 0;

+ 1 - 0
panda/src/express/subStreamBuf.h

@@ -23,6 +23,7 @@
 class EXPCL_PANDA_EXPRESS SubStreamBuf : public std::streambuf {
 public:
   SubStreamBuf();
+  SubStreamBuf(const SubStreamBuf &copy) = delete;
   virtual ~SubStreamBuf();
 
   void open(IStreamWrapper *source, OStreamWrapper *dest, std::streampos start, std::streampos end, bool append);