From 5d2ed0326ed27f32c4a8ec5e95ec9fc57139c7ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 12 Feb 2015 17:21:22 -0800 Subject: [PATCH 1/3] fix double-free in race conditions when notify() was called twice --- src/mbgl/storage/request.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index c1a57e72566..02c2822d764 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -19,7 +19,7 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) // thread (the thread notify() is called from) rather than kicking back to the calling thread. if (loop) { notify_async = new uv_async_t; - notify_async->data = this; + notify_async->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 uv_async_init(loop, notify_async, [](uv_async_t *async, int) { notifyCallback(async); }); #else @@ -57,6 +57,8 @@ Request::~Request() { void Request::notify(const std::shared_ptr &response_) { response = response_; if (notify_async) { + assert(!notify_async->data); + notify_async->data = this; uv_async_send(notify_async); } else { assert(response); @@ -70,7 +72,7 @@ void Request::cancel() { assert(notify_async); assert(!destruct_async); destruct_async = new uv_async_t; - destruct_async->data = this; + destruct_async->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 uv_async_init(notify_async->loop, destruct_async, [](uv_async_t *async, int) { cancelCallback(async); }); #else @@ -88,11 +90,9 @@ void Request::cancelCallback(uv_async_t *async) { // This gets called from the FileSource thread, and will only ever be invoked after cancel() was called // in the original requesting thread. void Request::destruct() { - if (notify_async) { - notify(nullptr); - } - assert(destruct_async); + assert(!destruct_async->data); + destruct_async->data = this; uv_async_send(destruct_async); } From aecef6da1a7903ef318e02f62d1cdd97a2e3e53b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 13 Feb 2015 10:54:36 -0800 Subject: [PATCH 2/3] make sure that the async handle always gets closed --- include/mbgl/storage/default/request.hpp | 3 +- src/mbgl/storage/request.cpp | 44 +++++++++++++++++------- test/storage/http_reading.cpp | 28 +++++++++++++++ 3 files changed, 62 insertions(+), 13 deletions(-) diff --git a/include/mbgl/storage/default/request.hpp b/include/mbgl/storage/default/request.hpp index 81ed14a5688..e26d59e2d9a 100644 --- a/include/mbgl/storage/default/request.hpp +++ b/include/mbgl/storage/default/request.hpp @@ -22,7 +22,6 @@ class Request : private util::noncopyable { public: using Callback = std::function; Request(const Resource &resource, uv_loop_t *loop, Callback callback); - ~Request(); public: // May be called from any thread. @@ -33,6 +32,8 @@ class Request : private util::noncopyable { void cancel(); private: + ~Request(); + void invoke(); static void notifyCallback(uv_async_t *async); static void cancelCallback(uv_async_t *async); diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index 02c2822d764..09d28ca0556 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -31,13 +31,15 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) void Request::notifyCallback(uv_async_t *async) { auto request = reinterpret_cast(async->data); uv::close(async); + assert(request); + MBGL_VERIFY_THREAD(request->tid) if (!request->destruct_async) { - // We haven't created a cancel request, so we can safely delete this Request object - // since it won't be accessed in the future. - assert(request->response); - request->callback(*request->response); - delete request; + // Call the callback with the result data. This will also delete this object. We haven't + // created a cancel request, so this is safe since it won't be accessed in the future. + // It is up to the user to not call cancel() on this Request object after the response was + // delivered. + request->invoke(); } else { // Otherwise, we're waiting for for the destruct notification to be delivered in order // to delete the Request object. We're doing this since we can't know whether the @@ -46,24 +48,30 @@ void Request::notifyCallback(uv_async_t *async) { } } +void Request::invoke() { + assert(response); + // The user could supply a null pointer or empty std::function as a callback. In this case, we + // still do the file request, but we don't need to deliver a result. + if (callback) { + callback(*response); + } + delete this; +} Request::~Request() { - if (notify_async) { - // Request objects can be destructed in other threads when the user didn't supply a loop. - MBGL_VERIFY_THREAD(tid) - } } void Request::notify(const std::shared_ptr &response_) { response = response_; + assert(response); if (notify_async) { assert(!notify_async->data); notify_async->data = this; uv_async_send(notify_async); } else { - assert(response); - callback(*response); - delete this; + // This request is not cancelable. This means that the callback will be executed in an + // arbitrary thread (== FileSource thread). + invoke(); } } @@ -84,13 +92,25 @@ void Request::cancelCallback(uv_async_t *async) { // The destruct_async will be invoked *after* the notify_async callback has already run. auto request = reinterpret_cast(async->data); uv::close(async); + assert(request); + MBGL_VERIFY_THREAD(request->tid) delete request; } // This gets called from the FileSource thread, and will only ever be invoked after cancel() was called // in the original requesting thread. void Request::destruct() { + assert(notify_async); assert(destruct_async); + + if (!notify_async->data) { + // The async hasn't been triggered yet, but we need to so that it'll close the handle. The + // callback will not delete this object since we have a destruct_async handle as well. + notify_async->data = this; + uv_async_send(notify_async); + } + + // This will finally destruct this object. assert(!destruct_async->data); destruct_async->data = this; uv_async_send(destruct_async); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index 3aa57cd805d..ad87db15ff6 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -37,3 +37,31 @@ TEST_F(Storage, HTTPReading) { uv_run(uv_default_loop(), UV_RUN_DEFAULT); } + +TEST_F(Storage, HTTPNoCallback) { + SCOPED_TEST(HTTPTest) + + using namespace mbgl; + + DefaultFileSource fs(nullptr, uv_default_loop()); + + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), nullptr); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + HTTPTest.finish(); +} + +TEST_F(Storage, HTTPNoCallbackNoLoop) { + SCOPED_TEST(HTTPTest) + + using namespace mbgl; + + DefaultFileSource fs(nullptr, uv_default_loop()); + + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, nullptr); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + HTTPTest.finish(); +} From f4e9908045ee4470ddfa7daac3bd241a801eca5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 13 Feb 2015 10:55:22 -0800 Subject: [PATCH 3/3] code style --- include/mbgl/storage/default/request.hpp | 4 +- src/mbgl/storage/request.cpp | 50 ++++++++++++------------ 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/include/mbgl/storage/default/request.hpp b/include/mbgl/storage/default/request.hpp index e26d59e2d9a..648585f304a 100644 --- a/include/mbgl/storage/default/request.hpp +++ b/include/mbgl/storage/default/request.hpp @@ -38,8 +38,8 @@ class Request : private util::noncopyable { static void cancelCallback(uv_async_t *async); private: - uv_async_t *notify_async = nullptr; - uv_async_t *destruct_async = nullptr; + uv_async_t *notifyAsync = nullptr; + uv_async_t *destructAsync = nullptr; Callback callback; std::shared_ptr response; diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index 09d28ca0556..de18138ec21 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -18,12 +18,12 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) // When there is no loop supplied (== nullptr), the callback will be fired in an arbitrary // thread (the thread notify() is called from) rather than kicking back to the calling thread. if (loop) { - notify_async = new uv_async_t; - notify_async->data = nullptr; + notifyAsync = new uv_async_t; + notifyAsync->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 - uv_async_init(loop, notify_async, [](uv_async_t *async, int) { notifyCallback(async); }); + uv_async_init(loop, notifyAsync, [](uv_async_t *async, int) { notifyCallback(async); }); #else - uv_async_init(loop, notify_async, notifyCallback); + uv_async_init(loop, notifyAsync, notifyCallback); #endif } } @@ -34,7 +34,7 @@ void Request::notifyCallback(uv_async_t *async) { assert(request); MBGL_VERIFY_THREAD(request->tid) - if (!request->destruct_async) { + if (!request->destructAsync) { // Call the callback with the result data. This will also delete this object. We haven't // created a cancel request, so this is safe since it won't be accessed in the future. // It is up to the user to not call cancel() on this Request object after the response was @@ -64,10 +64,10 @@ Request::~Request() { void Request::notify(const std::shared_ptr &response_) { response = response_; assert(response); - if (notify_async) { - assert(!notify_async->data); - notify_async->data = this; - uv_async_send(notify_async); + if (notifyAsync) { + assert(!notifyAsync->data); + notifyAsync->data = this; + uv_async_send(notifyAsync); } else { // This request is not cancelable. This means that the callback will be executed in an // arbitrary thread (== FileSource thread). @@ -77,19 +77,19 @@ void Request::notify(const std::shared_ptr &response_) { void Request::cancel() { MBGL_VERIFY_THREAD(tid) - assert(notify_async); - assert(!destruct_async); - destruct_async = new uv_async_t; - destruct_async->data = nullptr; + assert(notifyAsync); + assert(!destructAsync); + destructAsync = new uv_async_t; + destructAsync->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 - uv_async_init(notify_async->loop, destruct_async, [](uv_async_t *async, int) { cancelCallback(async); }); + uv_async_init(notifyAsync->loop, destructAsync, [](uv_async_t *async, int) { cancelCallback(async); }); #else - uv_async_init(notify_async->loop, destruct_async, cancelCallback); + uv_async_init(notifyAsync->loop, destructAsync, cancelCallback); #endif } void Request::cancelCallback(uv_async_t *async) { - // The destruct_async will be invoked *after* the notify_async callback has already run. + // The destructAsync will be invoked *after* the notifyAsync callback has already run. auto request = reinterpret_cast(async->data); uv::close(async); assert(request); @@ -100,20 +100,20 @@ void Request::cancelCallback(uv_async_t *async) { // This gets called from the FileSource thread, and will only ever be invoked after cancel() was called // in the original requesting thread. void Request::destruct() { - assert(notify_async); - assert(destruct_async); + assert(notifyAsync); + assert(destructAsync); - if (!notify_async->data) { + if (!notifyAsync->data) { // The async hasn't been triggered yet, but we need to so that it'll close the handle. The - // callback will not delete this object since we have a destruct_async handle as well. - notify_async->data = this; - uv_async_send(notify_async); + // callback will not delete this object since we have a destructAsync handle as well. + notifyAsync->data = this; + uv_async_send(notifyAsync); } // This will finally destruct this object. - assert(!destruct_async->data); - destruct_async->data = this; - uv_async_send(destruct_async); + assert(!destructAsync->data); + destructAsync->data = this; + uv_async_send(destructAsync); } }