folly: ubsan: replace undefined call through reinterpret-cast fn-pointer with std::function

Summary:
This code casts function pointers to void(*fn)(void*) and calls
functions through that type. Calling functions through a different
pointer type is undefined behavior. Casts were used to avoid memory
allocations.

`std::bind` needs a memory allocation - so it's avoided in EventBase.

  std::function<void()> x = std::bind(fn, args);

`std::function` should use small object optimizations when possible and embed the functor in the body of `std::function`.

On GCC 5.0 and forward small lambdas don't need memory allocations
(lambdas being tiny structures - two pointers in this case - and a
function operator).

  std::function<void()> y = [=] { fn(args); };

On GCC 4.7 .. 4.9 functors for which __is_location_invariant<Func> is
true also don't need memory allocations.

Remove undefined behavior by using a `SmallFunctor` which leverages `std::function`'s small object optimization.

Reviewed By: philippv

Differential Revision: D2864895

fb-gh-sync-id: ab40f60b3519ce38f43fecebf88ccdbf09d9bea9

folly/io/async/EventBase.cpp

@@ -41,8 +41,7 @@ class FunctionLoopCallback : public EventBase::LoopCallback {
   explicit FunctionLoopCallback(Cob&& function)
       : function_(std::move(function)) {}
 
-  explicit FunctionLoopCallback(const Cob& function)
-      : function_(function) {}
+  explicit FunctionLoopCallback(const Cob& function) : function_(function) {}
 
   void runLoopCallback() noexcept override {
     function_();
@@ -52,7 +51,6 @@ class FunctionLoopCallback : public EventBase::LoopCallback {
  private:
   Callback function_;
 };
-
 }
 
 namespace folly {
@@ -63,10 +61,9 @@ const int kNoFD = -1;
  * EventBase::FunctionRunner
  */
 
-class EventBase::FunctionRunner
-    : public NotificationQueue<std::pair<void (*)(void*), void*>>::Consumer {
+class EventBase::FunctionRunner : public NotificationQueue<Cob>::Consumer {
  public:
-  void messageAvailable(std::pair<void (*)(void*), void*>&& msg) override {
+  void messageAvailable(Cob&& msg) override {
 
     // In libevent2, internal events do not break the loop.
     // Most users would expect loop(), followed by runInEventBaseThread(),
@@ -76,25 +73,18 @@ class EventBase::FunctionRunner
     // stop_ flag as well as runInLoop callbacks, etc.
     event_base_loopbreak(getEventBase()->evb_);
 
-    if (msg.first == nullptr && msg.second == nullptr) {
+    if (!msg) {
       // terminateLoopSoon() sends a null message just to
       // wake up the loop.  We can ignore these messages.
       return;
     }
 
-    // If function is nullptr, just log and move on
-    if (!msg.first) {
-      LOG(ERROR) << "nullptr callback registered to be run in "
-                 << "event base thread";
-      return;
-    }
-
     // The function should never throw an exception, because we have no
     // way of knowing what sort of error handling to perform.
     //
     // If it does throw, log a message and abort the program.
     try {
-      msg.first(msg.second);
+      msg();
     } catch (const std::exception& ex) {
       LOG(ERROR) << "runInEventBaseThread() function threw a "
                  << typeid(ex).name() << " exception: " << ex.what();
@@ -495,7 +485,7 @@ void EventBase::terminateLoopSoon() {
   // this likely means the EventBase already has lots of events waiting
   // anyway.
   try {
-    queue_->putMessage(std::make_pair(nullptr, nullptr));
+    queue_->putMessage(nullptr);
   } catch (...) {
     // We don't care if putMessage() fails.  This likely means
     // the EventBase already has lots of events waiting anyway.
@@ -554,7 +544,7 @@ void EventBase::runBeforeLoop(LoopCallback* callback) {
   runBeforeLoopCallbacks_.push_back(*callback);
 }
 
-bool EventBase::runInEventBaseThread(void (*fn)(void*), void* arg) {
+bool EventBase::runInEventBaseThread(const Cob& fn) {
   // Send the message.
   // It will be received by the FunctionRunner in the EventBase's thread.
 
@@ -567,42 +557,16 @@ bool EventBase::runInEventBaseThread(void (*fn)(void*), void* arg) {
 
   // Short-circuit if we are already in our event base
   if (inRunningEventBaseThread()) {
-    runInLoop(new RunInLoopCallback(fn, arg));
+    runInLoop(fn);
     return true;
 
   }
 
   try {
-    queue_->putMessage(std::make_pair(fn, arg));
+    queue_->putMessage(fn);
   } catch (const std::exception& ex) {
     LOG(ERROR) << "EventBase " << this << ": failed to schedule function "
-               << fn << "for EventBase thread: " << ex.what();
-    return false;
-  }
-
-  return true;
-}
-
-bool EventBase::runInEventBaseThread(const Cob& fn) {
-  // Short-circuit if we are already in our event base
-  if (inRunningEventBaseThread()) {
-    runInLoop(fn);
-    return true;
-  }
-
-  Cob* fnCopy;
-  // Allocate a copy of the function so we can pass it to the other thread
-  // The other thread will delete this copy once the function has been run
-  try {
-    fnCopy = new Cob(fn);
-  } catch (const std::bad_alloc& ex) {
-    LOG(ERROR) << "failed to allocate tr::function copy "
-               << "for runInEventBaseThread()";
-    return false;
-  }
-
-  if (!runInEventBaseThread(&EventBase::runFunctionPtr, fnCopy)) {
-    delete fnCopy;
+               << "for EventBase thread: " << ex.what();
     return false;
   }
 
@@ -698,7 +662,7 @@ bool EventBase::runLoopCallbacks(bool setContext) {
 
 void EventBase::initNotificationQueue() {
   // Infinite size queue
-  queue_.reset(new NotificationQueue<std::pair<void (*)(void*), void*>>());
+  queue_.reset(new NotificationQueue<Cob>());
 
   // We allocate fnRunner_ separately, rather than declaring it directly
   // as a member of EventBase solely so that we don't need to include
@@ -778,15 +742,6 @@ void EventBase::runFunctionPtr(Cob* fn) {
   delete fn;
 }
 
-EventBase::RunInLoopCallback::RunInLoopCallback(void (*fn)(void*), void* arg)
-    : fn_(fn)
-    , arg_(arg) {}
-
-void EventBase::RunInLoopCallback::runLoopCallback() noexcept {
-  fn_(arg_);
-  delete this;
-}
-
 void EventBase::attachTimeoutManager(AsyncTimeout* obj,
                                       InternalEnum internal) {
 

folly/io/async/EventBase.h

@@ -16,30 +16,33 @@
 
 #pragma once
 
-#include <glog/logging.h>
-#include <folly/io/async/AsyncTimeout.h>
-#include <folly/io/async/TimeoutManager.h>
-#include <folly/io/async/Request.h>
-#include <folly/Executor.h>
-#include <folly/experimental/ExecutionObserver.h>
-#include <folly/futures/DrivableExecutor.h>
-#include <memory>
-#include <stack>
+#include <atomic>
+#include <cstdlib>
+#include <errno.h>
+#include <functional>
 #include <list>
+#include <math.h>
+#include <memory>
+#include <mutex>
 #include <queue>
-#include <cstdlib>
 #include <set>
-#include <unordered_set>
+#include <stack>
 #include <unordered_map>
-#include <mutex>
+#include <unordered_set>
 #include <utility>
+
 #include <boost/intrusive/list.hpp>
 #include <boost/utility.hpp>
-#include <functional>
+#include <folly/Executor.h>
+#include <folly/Portability.h>
+#include <folly/experimental/ExecutionObserver.h>
+#include <folly/futures/DrivableExecutor.h>
+#include <folly/io/async/AsyncTimeout.h>
+#include <folly/io/async/Request.h>
+#include <folly/io/async/TimeoutManager.h>
+#include <glog/logging.h>
+
 #include <event.h>  // libevent
-#include <errno.h>
-#include <math.h>
-#include <atomic>
 
 namespace folly {
 
@@ -359,13 +362,8 @@ class EventBase : private boost::noncopyable,
    * @return Returns true if the function was successfully scheduled, or false
    *         if there was an error scheduling the function.
    */
-  template<typename T>
-  bool runInEventBaseThread(void (*fn)(T*), T* arg) {
-    return runInEventBaseThread(reinterpret_cast<void (*)(void*)>(fn),
-                                reinterpret_cast<void*>(arg));
-  }
-
-  bool runInEventBaseThread(void (*fn)(void*), void* arg);
+  template <typename T>
+  bool runInEventBaseThread(void (*fn)(T*), T* arg);
 
   /**
    * Run the specified function in the EventBase's thread
@@ -387,19 +385,8 @@ class EventBase : private boost::noncopyable,
    * Like runInEventBaseThread, but the caller waits for the callback to be
    * executed.
    */
-  template<typename T>
-  bool runInEventBaseThreadAndWait(void (*fn)(T*), T* arg) {
-    return runInEventBaseThreadAndWait(reinterpret_cast<void (*)(void*)>(fn),
-                                       reinterpret_cast<void*>(arg));
-  }
-
-  /*
-   * Like runInEventBaseThread, but the caller waits for the callback to be
-   * executed.
-   */
-  bool runInEventBaseThreadAndWait(void (*fn)(void*), void* arg) {
-    return runInEventBaseThreadAndWait(std::bind(fn, arg));
-  }
+  template <typename T>
+  bool runInEventBaseThreadAndWait(void (*fn)(T*), T* arg);
 
   /*
    * Like runInEventBaseThread, but the caller waits for the callback to be
@@ -411,20 +398,8 @@ class EventBase : private boost::noncopyable,
    * Like runInEventBaseThreadAndWait, except if the caller is already in the
    * event base thread, the functor is simply run inline.
    */
-  template<typename T>
-  bool runImmediatelyOrRunInEventBaseThreadAndWait(void (*fn)(T*), T* arg) {
-    return runImmediatelyOrRunInEventBaseThreadAndWait(
-        reinterpret_cast<void (*)(void*)>(fn), reinterpret_cast<void*>(arg));
-  }
-
-  /*
-   * Like runInEventBaseThreadAndWait, except if the caller is already in the
-   * event base thread, the functor is simply run inline.
-   */
-  bool runImmediatelyOrRunInEventBaseThreadAndWait(
-      void (*fn)(void*), void* arg) {
-    return runImmediatelyOrRunInEventBaseThreadAndWait(std::bind(fn, arg));
-  }
+  template <typename T>
+  bool runImmediatelyOrRunInEventBaseThreadAndWait(void (*fn)(T*), T* arg);
 
   /*
    * Like runInEventBaseThreadAndWait, except if the caller is already in the
@@ -610,7 +585,6 @@ class EventBase : private boost::noncopyable,
   }
 
  private:
-
   // TimeoutManager
   void attachTimeoutManager(AsyncTimeout* obj,
                             TimeoutManager::InternalEnum internal) override;
@@ -626,17 +600,6 @@ class EventBase : private boost::noncopyable,
     return isInEventBaseThread();
   }
 
-  // Helper class used to short circuit runInEventBaseThread
-  class RunInLoopCallback : public LoopCallback {
-   public:
-    RunInLoopCallback(void (*fn)(void*), void* arg);
-    void runLoopCallback() noexcept;
-
-   private:
-    void (*fn_)(void*);
-    void* arg_;
-  };
-
   /*
    * Helper function that tells us whether we have already handled
    * some event/timeout/callback in this loop iteration.
@@ -645,7 +608,7 @@ class EventBase : private boost::noncopyable,
 
   // --------- libevent callbacks (not for client use) ------------
 
-  static void runFunctionPtr(std::function<void()>* fn);
+  static void runFunctionPtr(Cob* fn);
 
   // small object used as a callback arg with enough info to execute the
   // appropriate client-provided Cob
@@ -710,7 +673,7 @@ class EventBase : private boost::noncopyable,
 
   // A notification queue for runInEventBaseThread() to use
   // to send function requests to the EventBase thread.
-  std::unique_ptr<NotificationQueue<std::pair<void (*)(void*), void*>>> queue_;
+  std::unique_ptr<NotificationQueue<Cob>> queue_;
   std::unique_ptr<FunctionRunner> fnRunner_;
 
   // limit for latency in microseconds (0 disables)
@@ -765,4 +728,80 @@ class EventBase : private boost::noncopyable,
   std::unordered_set<detail::EventBaseLocalBaseBase*> localStorageToDtor_;
 };
 
+namespace detail {
+
+/**
+ * Define a small functor (2 pointers) and specialize
+ * std::__is_location_invariant so that std::function does not require
+ * memory allocation.
+ *
+ *   std::function<void()> func = SmallFunctor{f, p};
+ *
+ * TODO(lucian): remove this hack once GCC <= 4.9 are deprecated.
+ * In GCC >= 5.0 just use a lambda like:
+ *
+ *   std::function<void()> func = [=] { f(p); };
+ *
+ * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61909
+ */
+template <class T>
+struct SmallFunctor {
+  void (*fn)(T*);
+  T* p;
+  void operator()() { fn(p); }
+};
+
+} // detail
+
+template <typename T>
+bool EventBase::runInEventBaseThread(void (*fn)(T*), T* arg) {
+  return runInEventBaseThread(detail::SmallFunctor<T>{fn, arg});
+}
+
+template <typename T>
+bool EventBase::runInEventBaseThreadAndWait(void (*fn)(T*), T* arg) {
+  return runInEventBaseThreadAndWait(detail::SmallFunctor<T>{fn, arg});
+}
+
+template <typename T>
+bool EventBase::runImmediatelyOrRunInEventBaseThreadAndWait(void (*fn)(T*),
+                                                            T* arg) {
+  return runImmediatelyOrRunInEventBaseThreadAndWait(
+      detail::SmallFunctor<T>{fn, arg});
+}
+
 } // folly
+
+FOLLY_NAMESPACE_STD_BEGIN
+
+/**
+ * GCC's libstdc++ uses __is_location_invariant to decide wether to
+ * use small object optimization and embed the functor's contents in
+ * the std::function object.
+ *
+ * (gcc 4.9) $ libstdc++-v3/include/std/functional
+ * template<typename _Tp>
+ *   struct __is_location_invariant
+ *   : integral_constant<bool, (is_pointer<_Tp>::value
+ *                              || is_member_pointer<_Tp>::value)>
+ *   { };
+ *
+ * (gcc 5.0) $ libstdc++-v3/include/std/functional
+ *
+ * template<typename _Tp>
+ *      struct __is_location_invariant
+ *      : is_trivially_copyable<_Tp>::type
+ *      { };
+ *
+ *
+ *  NOTE: Forward declare so this doesn't break when using other
+ *  standard libraries: it just wont have any effect.
+ */
+template <typename T>
+struct __is_location_invariant;
+
+template <typename T>
+struct __is_location_invariant<folly::detail::SmallFunctor<T>>
+    : public std::true_type {};
+
+FOLLY_NAMESPACE_STD_END