From 2a80579ea13d70f0750e10d950785be3046ada96 Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Tue, 9 Jun 2020 12:37:54 -0700
Subject: [PATCH] Fabric: Checking for `nullptr` before calling `CFRelease` in
 `ManagedObjectWrapper`

Summary:
ManagedObjectWrapper (`wrapManagedObject`) is used to pass pointers managed by Objective-C runtime object thought C++ parts of the framework. The wrapper consists of a shared pointer to an object with a custom deleter that calls `CFRelease`.
Apparently, there is a caveat here: shared ptr implementation always calls a deleter even if the object points to `nullptr`. It's usually fine because in C++ calling `delete` on a nullptr is a no-op, not an error. But it's an error from the `CFRelease` perspective, which checks the pointer and crashes if it's nullptr.

More info:
https://stackoverflow.com/questions/1135214/why-would-cfreleasenull-crash
https://stackoverflow.com/questions/50201967/why-unique-ptr-with-custom-deleter-wont-work-for-nullptr-while-shared-ptr-does

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D21943011

fbshipit-source-id: 442ad5e274a146de112e6bd8f3c2d20f0225bf77
---
 ReactCommon/utils/ManagedObjectWrapper.h | 31 ++++++++++++++++++++----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/ReactCommon/utils/ManagedObjectWrapper.h b/ReactCommon/utils/ManagedObjectWrapper.h
index 0f44838c39f..005638d087e 100644
--- a/ReactCommon/utils/ManagedObjectWrapper.h
+++ b/ReactCommon/utils/ManagedObjectWrapper.h
@@ -21,6 +21,27 @@
 namespace facebook {
 namespace react {
 
+namespace detail {
+
+/*
+ * A custom deleter used for the deallocation of Objective-C managed objects.
+ * To be used only by `wrapManagedObject`.
+ */
+static void wrappedManagedObjectDeleter(void *cfPointer) noexcept
+{
+  // A shared pointer does call custom deleter on `nullptr`s.
+  // This is somewhat counter-intuitively but makes sense considering the type-erasured nature of shared pointer and an
+  // aliasing constructor feature. `CFRelease` crashes on null pointer though. Therefore we must check for this case
+  // explicitly.
+  if (cfPointer == NULL) {
+    return;
+  }
+
+  CFRelease(cfPointer);
+}
+
+}
+
 /*
  * `wrapManagedObject` and `unwrapManagedObject` are wrapper functions that
  * convert ARC-managed objects into `std::shared_ptr<void>` and vice-versa. It's
@@ -35,24 +56,24 @@ namespace react {
  * represented as multiple bumps of C++ counter, so we can have multiple
  * counters for the same object that form some kind of counters tree.
  */
-inline std::shared_ptr<void> wrapManagedObject(id object)
+inline std::shared_ptr<void> wrapManagedObject(id object) noexcept
 {
-  return std::shared_ptr<void>((__bridge_retained void *)object, CFRelease);
+  return std::shared_ptr<void>((__bridge_retained void *)object, detail::wrappedManagedObjectDeleter);
 }
 
-inline id unwrapManagedObject(std::shared_ptr<void> const &object)
+inline id unwrapManagedObject(std::shared_ptr<void> const &object) noexcept
 {
   return (__bridge id)object.get();
 }
 
-inline std::shared_ptr<void> wrapManagedObjectWeakly(id object)
+inline std::shared_ptr<void> wrapManagedObjectWeakly(id object) noexcept
 {
   RCTInternalGenericWeakWrapper *weakWrapper = [RCTInternalGenericWeakWrapper new];
   weakWrapper.object = object;
   return wrapManagedObject(weakWrapper);
 }
 
-inline id unwrapManagedObjectWeakly(std::shared_ptr<void> const &object)
+inline id unwrapManagedObjectWeakly(std::shared_ptr<void> const &object) noexcept
 {
   RCTInternalGenericWeakWrapper *weakWrapper = (RCTInternalGenericWeakWrapper *)unwrapManagedObject(object);
   assert(weakWrapper && "`RCTInternalGenericWeakWrapper` instance must not be `nil`.");