Merge pull request #876 from mapbox/fix-request-double-free

Fix request double free

include/mbgl/storage/default/request.hpp

@@ -22,7 +22,6 @@ class Request : private util::noncopyable {
 public:
     using Callback = std::function<void(const Response &)>;
     Request(const Resource &resource, uv_loop_t *loop, Callback callback);
-    ~Request();
 
 public:
     // May be called from any thread.
@@ -33,12 +32,14 @@ public:
     void cancel();
 
 private:
+    ~Request();
+    void invoke();
     static void notifyCallback(uv_async_t *async);
     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<const Response> response;
 

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 = this;
+        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
     }
 }
@@ -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<Request *>(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;
+    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
+        // 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,54 +48,72 @@ 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<const Response> &response_) {
     response = response_;
-    if (notify_async) {
-        uv_async_send(notify_async);
+    assert(response);
+    if (notifyAsync) {
+        assert(!notifyAsync->data);
+        notifyAsync->data = this;
+        uv_async_send(notifyAsync);
     } 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();
     }
 }
 
 void Request::cancel() {
     MBGL_VERIFY_THREAD(tid)
-    assert(notify_async);
-    assert(!destruct_async);
-    destruct_async = new uv_async_t;
-    destruct_async->data = this;
+    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<Request *>(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() {
-    if (notify_async) {
-        notify(nullptr);
+    assert(notifyAsync);
+    assert(destructAsync);
+
+    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 destructAsync handle as well.
+        notifyAsync->data = this;
+        uv_async_send(notifyAsync);
     }
 
-    assert(destruct_async);
-    uv_async_send(destruct_async);
+    // This will finally destruct this object.
+    assert(!destructAsync->data);
+    destructAsync->data = this;
+    uv_async_send(destructAsync);
 }
 
 }

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();
+}