Bug 1596198 - Remove the canary from nsStringBuffer r=bzbarsky
authorPaul Bone <pbone@mozilla.com>
Thu, 14 Nov 2019 22:50:31 +0000
changeset 502084 152f204eda63b35bb0d12b989141dd7261d32452
parent 502083 fa6c87627ec8d357de398ebdbb05bc03399b8df4
child 502085 ff0b20be880be24af649b8b776608a2672346615
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1596198
milestone72.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 1596198 - Remove the canary from nsStringBuffer r=bzbarsky Differential Revision: https://phabricator.services.mozilla.com/D52961
xpcom/string/nsStringBuffer.h
xpcom/string/nsSubstring.cpp
--- a/xpcom/string/nsStringBuffer.h
+++ b/xpcom/string/nsStringBuffer.h
@@ -8,52 +8,32 @@
 #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.
  */
 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
    * buffer by calling the Data method to fetch the raw data pointer.  Care
@@ -91,21 +71,17 @@ class nsStringBuffer {
   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.
    */
   static nsStringBuffer* FromData(void* aData) {
-    nsStringBuffer* sb = reinterpret_cast<nsStringBuffer*>(aData) - 1;
-#ifdef STRING_BUFFER_CANARY
-    if (MOZ_UNLIKELY(sb->mCanary != CANARY_OK)) sb->FromDataCanaryCheckFailed();
-#endif
-    return sb;
+    return reinterpret_cast<nsStringBuffer*>(aData) - 1;
   }
 
   /**
    * This method returns the data pointer for this string buffer.
    */
   void* Data() const {
     return const_cast<char*>(reinterpret_cast<const char*>(this + 1));
   }
@@ -189,20 +165,11 @@ class nsStringBuffer {
    *
    * WARNING: Only use this if you really know what you are doing, because
    * it can easily lead to double-counting strings.  If you do use them,
    * please explain clearly in a comment why it's safe and won't lead to
    * double-counting.
    */
   size_t SizeOfIncludingThisEvenIfShared(
       mozilla::MallocSizeOf aMallocSizeOf) const;
-
-#ifdef STRING_BUFFER_CANARY
-  /*
-   * Called by FromData if the canary check failed.  This is out-of-line in
-   * nsSubstring.cpp so that MOZ_CRASH_UNSAFE_PRINTF is available via #includes.
-   * It is not available in FromData due to #include-order.
-   */
-  void FromDataCanaryCheckFailed() const;
-#endif
 };
 
 #endif /* !defined(nsStringBuffer_h__ */
--- a/xpcom/string/nsSubstring.cpp
+++ b/xpcom/string/nsSubstring.cpp
@@ -15,45 +15,31 @@
 #  include <stdio.h>
 #endif
 
 #include <stdlib.h>
 #include "nsAString.h"
 #include "nsString.h"
 #include "nsStringBuffer.h"
 #include "nsDependentString.h"
-#include "nsPrintfCString.h"
 #include "nsMemory.h"
 #include "prprf.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) {                                \
-        MOZ_CRASH_UNSAFE_PRINTF("Bad canary value 0x%x", c); \
-      }                                                      \
-    } 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);
@@ -182,33 +168,28 @@ void nsStringBuffer::AddRef() {
       + 1
 #endif
       ;
   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.
@@ -220,28 +201,24 @@ already_AddRefed<nsStringBuffer> nsStrin
                "mStorageSize will truncate");
 
   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");
 
@@ -320,22 +297,16 @@ size_t nsStringBuffer::SizeOfIncludingTh
   return IsReadonly() ? 0 : aMallocSizeOf(this);
 }
 
 size_t nsStringBuffer::SizeOfIncludingThisEvenIfShared(
     mozilla::MallocSizeOf aMallocSizeOf) const {
   return aMallocSizeOf(this);
 }
 
-#ifdef STRING_BUFFER_CANARY
-void nsStringBuffer::FromDataCanaryCheckFailed() const {
-  MOZ_CRASH_UNSAFE_PRINTF("Bad canary value 0x%x in FromData", mCanary);
-}
-#endif
-
 // ---------------------------------------------------------------------------
 
 // define nsAString
 #include "nsTSubstring.cpp"
 
 // Provide rust bindings to the nsA[C]String types
 extern "C" {