From e2c9fd6c6f57777bf9d3fe09038bc714124d2060 Mon Sep 17 00:00:00 2001 From: Lucian Grijincu Date: Thu, 28 Jan 2016 11:07:44 -0800 Subject: [PATCH] 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 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 y = [=] { fn(args); }; On GCC 4.7 .. 4.9 functors for which __is_location_invariant 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 | 67 +++----------- folly/io/async/EventBase.h | 167 +++++++++++++++++++++-------------- 2 files changed, 114 insertions(+), 120 deletions(-) diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp index abd93dc1..c636b0ee 100644 --- a/folly/io/async/EventBase.cpp +++ b/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>::Consumer { +class EventBase::FunctionRunner : public NotificationQueue::Consumer { public: - void messageAvailable(std::pair&& 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>()); + queue_.reset(new NotificationQueue()); // 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) { diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h index 3cdaf9a7..7167e4a3 100644 --- a/folly/io/async/EventBase.h +++ b/folly/io/async/EventBase.h @@ -16,30 +16,33 @@ #pragma once -#include -#include -#include -#include -#include -#include -#include -#include -#include +#include +#include +#include +#include #include +#include +#include +#include #include -#include #include -#include +#include #include -#include +#include #include + #include #include -#include +#include +#include +#include +#include +#include +#include +#include +#include + #include // libevent -#include -#include -#include 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 - bool runInEventBaseThread(void (*fn)(T*), T* arg) { - return runInEventBaseThread(reinterpret_cast(fn), - reinterpret_cast(arg)); - } - - bool runInEventBaseThread(void (*fn)(void*), void* arg); + template + 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 - bool runInEventBaseThreadAndWait(void (*fn)(T*), T* arg) { - return runInEventBaseThreadAndWait(reinterpret_cast(fn), - reinterpret_cast(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 + 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 - bool runImmediatelyOrRunInEventBaseThreadAndWait(void (*fn)(T*), T* arg) { - return runImmediatelyOrRunInEventBaseThreadAndWait( - reinterpret_cast(fn), reinterpret_cast(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 + 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* 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>> queue_; + std::unique_ptr> queue_; std::unique_ptr fnRunner_; // limit for latency in microseconds (0 disables) @@ -765,4 +728,80 @@ class EventBase : private boost::noncopyable, std::unordered_set 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 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 func = [=] { f(p); }; + * + * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61909 + */ +template +struct SmallFunctor { + void (*fn)(T*); + T* p; + void operator()() { fn(p); } +}; + +} // detail + +template +bool EventBase::runInEventBaseThread(void (*fn)(T*), T* arg) { + return runInEventBaseThread(detail::SmallFunctor{fn, arg}); +} + +template +bool EventBase::runInEventBaseThreadAndWait(void (*fn)(T*), T* arg) { + return runInEventBaseThreadAndWait(detail::SmallFunctor{fn, arg}); +} + +template +bool EventBase::runImmediatelyOrRunInEventBaseThreadAndWait(void (*fn)(T*), + T* arg) { + return runImmediatelyOrRunInEventBaseThreadAndWait( + detail::SmallFunctor{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 + * struct __is_location_invariant + * : integral_constant::value + * || is_member_pointer<_Tp>::value)> + * { }; + * + * (gcc 5.0) $ libstdc++-v3/include/std/functional + * + * template + * 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 +struct __is_location_invariant; + +template +struct __is_location_invariant> + : public std::true_type {}; + +FOLLY_NAMESPACE_STD_END -- 2.34.1