Bug 1410276 - Add a canary field to nsStringBuffer. r=bz
authorPaul Bone <pbone@mozilla.com>
Wed, 25 Oct 2017 23:21:47 +1100
changeset 388250 a3785ec9a48c8c76dd98b5c5140283f8ac44c851
parent 388249 d376cac28778edfaf3ba95f12bda3df7a9cecefe
child 388251 c2cecb6d6f9bb85f258a66cf8e94c1860adde7e5
child 388423 d734e6acf7778df7c933d33540203f08b44ff977
push id96595
push userryanvm@gmail.com
push dateThu, 26 Oct 2017 01:21:33 +0000
treeherdermozilla-inbound@a3785ec9a48c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1410276
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1410276 - Add a canary field to nsStringBuffer. r=bz
xpcom/string/nsStringBuffer.h
xpcom/string/nsSubstring.cpp
--- a/xpcom/string/nsStringBuffer.h
+++ b/xpcom/string/nsStringBuffer.h
@@ -7,16 +7,32 @@
 #ifndef nsStringBuffer_h__
 #define nsStringBuffer_h__
 
 #include <atomic>
 #include "mozilla/MemoryReporting.h"
 
 template<class T> struct already_AddRefed;
 
+/*
+ * Add a canary field to protect against double-frees of nsStringBuffer and
+ * other potential heap corruptions.  We intend to back this out before 58 hits
+ * beta.
+ */
+#if (defined(DEBUG) || defined(NIGHTLY_BUILD)) && !defined(MOZ_ASAN)
+# define STRING_BUFFER_CANARY 1
+#endif
+
+#ifdef STRING_BUFFER_CANARY
+enum nsStringBufferCanary : uint32_t {
+  CANARY_OK = 0xaf57c8fa,
+  CANARY_POISON = 0x534dc0f5
+};
+#endif
+
 /**
  * This structure precedes the string buffers "we" allocate.  It may be the
  * case that nsTAString::mData does not point to one of these special
  * buffers.  The mDataFlags member variable distinguishes the buffer type.
  *
  * When this header is in use, it enables reference counting, and capacity
  * tracking.  NOTE: A string buffer can be modified only if its reference
  * count is 1.
@@ -24,16 +40,20 @@ template<class T> struct already_AddRefe
 class nsStringBuffer
 {
 private:
   friend class CheckStaticAtomSizes;
 
   std::atomic<uint32_t> mRefCount;
   uint32_t mStorageSize;
 
+#ifdef STRING_BUFFER_CANARY
+  uint32_t mCanary;
+#endif
+
 public:
 
   /**
    * Allocates a new string buffer, with given size in bytes and a
    * reference count of one.  When the string buffer is no longer needed,
    * it should be released via Release.
    *
    * It is up to the caller to set the bytes corresponding to the string
@@ -70,21 +90,22 @@ public:
    * buffer will be destroyed when its reference count reaches zero.
    */
   void NS_FASTCALL Release();
 
   /**
    * This method returns the string buffer corresponding to the given data
    * pointer.  The data pointer must have been returned previously by a
    * call to the nsStringBuffer::Data method.
+   *
+   * This method is normally defined here in the header file for inlining into
+   * callers.  It has been outlined temporarily to make the canary checking code
+   * simpler WRT header include order.
    */
-  static nsStringBuffer* FromData(void* aData)
-  {
-    return reinterpret_cast<nsStringBuffer*>(aData) - 1;
-  }
+  static nsStringBuffer* FromData(void* aData);
 
   /**
    * This method returns the data pointer for this string buffer.
    */
   void* Data() const
   {
     return const_cast<char*>(reinterpret_cast<const char*>(this + 1));
   }
--- a/xpcom/string/nsSubstring.cpp
+++ b/xpcom/string/nsSubstring.cpp
@@ -15,32 +15,46 @@
 #include <stdio.h>
 #endif
 
 #include <stdlib.h>
 #include "nsSubstring.h"
 #include "nsString.h"
 #include "nsStringBuffer.h"
 #include "nsDependentString.h"
+#include "nsPrintfCString.h"
 #include "nsMemory.h"
 #include "prprf.h"
 #include "nsStaticAtom.h"
 #include "nsCOMPtr.h"
 
 #include "mozilla/IntegerPrintfMacros.h"
 #ifdef XP_WIN
 #include <windows.h>
 #include <process.h>
 #define getpid() _getpid()
 #define pthread_self() GetCurrentThreadId()
 #else
 #include <pthread.h>
 #include <unistd.h>
 #endif
 
+#ifdef STRING_BUFFER_CANARY
+#define CHECK_STRING_BUFFER_CANARY(c)                                     \
+  do {                                                                    \
+    if ((c) != CANARY_OK) {                                               \
+      NS_RUNTIMEABORT(nsPrintfCString("Bad canary value %d", c).get());   \
+    }                                                                     \
+  } while(0)
+#else
+#define CHECK_STRING_BUFFER_CANARY(c)                                     \
+  do {                                                                    \
+  } while(0)
+#endif
+
 using mozilla::Atomic;
 
 // ---------------------------------------------------------------------------
 
 static const char16_t gNullChar = 0;
 
 char* const nsCharTraits<char>::sEmptyBuffer =
   (char*)const_cast<char16_t*>(&gNullChar);
@@ -200,28 +214,33 @@ nsStringBuffer::AddRef()
     ;
   STRING_STAT_INCREMENT(Share);
   NS_LOG_ADDREF(this, count, "nsStringBuffer", sizeof(*this));
 }
 
 void
 nsStringBuffer::Release()
 {
+  CHECK_STRING_BUFFER_CANARY(mCanary);
+
   // Since this may be the last release on this thread, we need
   // release semantics so that prior writes on this thread are visible
   // to the thread that destroys the object when it reads mValue with
   // acquire semantics.
   uint32_t count = mRefCount.fetch_sub(1, std::memory_order_release) - 1;
   NS_LOG_RELEASE(this, count, "nsStringBuffer");
   if (count == 0) {
     // We're going to destroy the object on this thread, so we need
     // acquire semantics to synchronize with the memory released by
     // the last release on other threads, that is, to ensure that
     // writes prior to that release are now visible on this thread.
     count = mRefCount.load(std::memory_order_acquire);
+#ifdef STRING_BUFFER_CANARY
+    mCanary = CANARY_POISON;
+#endif
 
     STRING_STAT_INCREMENT(Free);
     free(this); // we were allocated with |malloc|
   }
 }
 
 /**
  * Alloc returns a pointer to a new string header with set capacity.
@@ -236,26 +255,30 @@ nsStringBuffer::Alloc(size_t aSize)
 
   nsStringBuffer* hdr =
     (nsStringBuffer*)malloc(sizeof(nsStringBuffer) + aSize);
   if (hdr) {
     STRING_STAT_INCREMENT(Alloc);
 
     hdr->mRefCount = 1;
     hdr->mStorageSize = aSize;
+#ifdef STRING_BUFFER_CANARY
+    hdr->mCanary = CANARY_OK;
+#endif
     NS_LOG_ADDREF(hdr, 1, "nsStringBuffer", sizeof(*hdr));
   }
   return dont_AddRef(hdr);
 }
 
 nsStringBuffer*
 nsStringBuffer::Realloc(nsStringBuffer* aHdr, size_t aSize)
 {
   STRING_STAT_INCREMENT(Realloc);
 
+  CHECK_STRING_BUFFER_CANARY(aHdr->mCanary);
   NS_ASSERTION(aSize != 0, "zero capacity allocation not allowed");
   NS_ASSERTION(sizeof(nsStringBuffer) + aSize <= size_t(uint32_t(-1)) &&
                sizeof(nsStringBuffer) + aSize > aSize,
                "mStorageSize will truncate");
 
   // no point in trying to save ourselves if we hit this assertion
   NS_ASSERTION(!aHdr->IsReadonly(), "|Realloc| attempted on readonly string");
 
@@ -269,16 +292,24 @@ nsStringBuffer::Realloc(nsStringBuffer* 
     NS_LOG_ADDREF(aHdr, 1, "nsStringBuffer", sizeof(*aHdr));
     aHdr->mStorageSize = aSize;
   }
 
   return aHdr;
 }
 
 nsStringBuffer*
+nsStringBuffer::FromData(void* aData)
+{
+  nsStringBuffer* str = reinterpret_cast<nsStringBuffer*>(aData) - 1;
+  CHECK_STRING_BUFFER_CANARY(str->mCanary);
+  return str;
+}
+
+nsStringBuffer*
 nsStringBuffer::FromString(const nsAString& aStr)
 {
   const nsAStringAccessor* accessor =
     static_cast<const nsAStringAccessor*>(&aStr);
 
   if (!(accessor->flags() & nsAString::DataFlags::SHARED)) {
     return nullptr;
   }