From cb3b77bc954ce27eb01ba80e10ea13a5888c22d7 Mon Sep 17 00:00:00 2001 From: Roman Rodyakin Date: Fri, 22 Nov 2019 09:05:49 -0800 Subject: [PATCH] Avoid crashing Flipper SDK on request for unsupported plugins Summary: in FlipperRSocketResponder::handleFireAndForget, a responder object was being conditionally (for requests with an "id" field) created, resulting in a null pointer passed into FlipperClient::onMessageReceived when no id was present. FlipperClient::onMessage received, in the meantime, unconditionally dereferences that pointer, resulting in a segmentation fault. This change creates the responder object regardless of whether or not the "id" key is present in the request object, thus avoiding passing a null pointer into FlipperClient::onMessageReceived. Reviewed By: jknoxville Differential Revision: D18583898 fbshipit-source-id: 2112c45bc0cd639cec908d0039d6bdaed2f61491 --- .../FireAndForgetBasedFlipperResponder.h | 24 +++++++++++++------ xplat/Flipper/FlipperRSocketResponder.cpp | 9 ++++--- .../FlipperConnectionManagerMock.h | 16 +++++++++++-- .../FlipperRSocketResponderTests.cpp | 6 +++-- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/xplat/Flipper/FireAndForgetBasedFlipperResponder.h b/xplat/Flipper/FireAndForgetBasedFlipperResponder.h index 1e0c7f7ff..f06da8e5a 100644 --- a/xplat/Flipper/FireAndForgetBasedFlipperResponder.h +++ b/xplat/Flipper/FireAndForgetBasedFlipperResponder.h @@ -7,9 +7,9 @@ #pragma once -#include "FlipperResponder.h" -#include "FlipperConnectionManager.h" #include +#include "FlipperConnectionManager.h" +#include "FlipperResponder.h" namespace facebook { namespace flipper { @@ -26,23 +26,33 @@ class FireAndForgetBasedFlipperResponder : public FlipperResponder { FireAndForgetBasedFlipperResponder( FlipperConnectionManager* socket, int64_t responseID) - : socket_(socket), responseID_(responseID) {} + : socket_(socket), responseID_(responseID), idValid_(true) {} + + FireAndForgetBasedFlipperResponder(FlipperConnectionManager* socket) + : socket_(socket), idValid_(false) {} void success(const folly::dynamic& response) override { - const folly::dynamic message = - folly::dynamic::object("id", responseID_)("success", response); + const folly::dynamic message = idValid_ + ? folly::dynamic::object("id", responseID_)("success", response) + : folly::dynamic::object("success", response); socket_->sendMessage(message); } void error(const folly::dynamic& response) override { - const folly::dynamic message = - folly::dynamic::object("id", responseID_)("error", response); + const folly::dynamic message = idValid_ + ? folly::dynamic::object("id", responseID_)("error", response) + : folly::dynamic::object("error", response); socket_->sendMessage(message); } + bool hasId() const { + return idValid_; + } + private: FlipperConnectionManager* socket_; int64_t responseID_; + bool idValid_; }; } // namespace flipper diff --git a/xplat/Flipper/FlipperRSocketResponder.cpp b/xplat/Flipper/FlipperRSocketResponder.cpp index 3f599df6c..d5e03cd48 100644 --- a/xplat/Flipper/FlipperRSocketResponder.cpp +++ b/xplat/Flipper/FlipperRSocketResponder.cpp @@ -26,10 +26,13 @@ void FlipperRSocketResponder::handleFireAndForget( const auto payload = request.moveDataToString(); std::unique_ptr responder; auto message = folly::parseJson(payload); - if (message.find("id") != message.items().end()) { - auto id = message["id"].getInt(); + auto idItr = message.find("id"); + if (idItr == message.items().end()) { responder = - std::make_unique(websocket_, id); + std::make_unique(websocket_); + } else { + responder = std::make_unique( + websocket_, idItr->second.getInt()); } websocket_->onMessageReceived( diff --git a/xplat/FlipperTestLib/FlipperConnectionManagerMock.h b/xplat/FlipperTestLib/FlipperConnectionManagerMock.h index 6bd4455d9..3403acc47 100644 --- a/xplat/FlipperTestLib/FlipperConnectionManagerMock.h +++ b/xplat/FlipperTestLib/FlipperConnectionManagerMock.h @@ -7,6 +7,7 @@ #pragma once +#include #include namespace facebook { @@ -43,7 +44,17 @@ class FlipperConnectionManagerMock : public FlipperConnectionManager { const folly::dynamic& message, std::unique_ptr responder) override { if (responder) { - respondersReceived++; + const FireAndForgetBasedFlipperResponder* const r = + dynamic_cast(responder.get()); + if (r) { + if (r->hasId()) { + ++respondersWithIdReceived; + } else { + ++respondersWithoutIdReceived; + } + } else { + ++respondersWithIdReceived; + } } callbacks->onMessageReceived(message, std::move(responder)); messagesReceived.push_back(message); @@ -58,7 +69,8 @@ class FlipperConnectionManagerMock : public FlipperConnectionManager { Callbacks* callbacks; std::vector messages; std::vector messagesReceived; - int respondersReceived = 0; + int respondersWithIdReceived = 0; + int respondersWithoutIdReceived = 0; }; } // namespace test diff --git a/xplat/FlipperTests/FlipperRSocketResponderTests.cpp b/xplat/FlipperTests/FlipperRSocketResponderTests.cpp index 257b639d3..31c2beeaa 100644 --- a/xplat/FlipperTests/FlipperRSocketResponderTests.cpp +++ b/xplat/FlipperTests/FlipperRSocketResponderTests.cpp @@ -44,7 +44,8 @@ TEST(FlipperRSocketResponderTests, testFireAndForgetWithoutIdParam) { responder.handleFireAndForget(rsocket::Payload(json), rsocket::StreamId(1)); EXPECT_EQ(socket.messagesReceived.size(), 1); EXPECT_EQ(socket.messagesReceived[0]["my"], "message"); - EXPECT_EQ(socket.respondersReceived, 0); + EXPECT_EQ(socket.respondersWithIdReceived, 0); + EXPECT_EQ(socket.respondersWithoutIdReceived, 1); } TEST(FlipperRSocketResponderTests, testFireAndForgetWithIdParam) { @@ -60,7 +61,8 @@ TEST(FlipperRSocketResponderTests, testFireAndForgetWithIdParam) { EXPECT_EQ(socket.messagesReceived.size(), 1); EXPECT_EQ(socket.messagesReceived[0]["my"], "message"); EXPECT_EQ(socket.messagesReceived[0]["id"], 7); - EXPECT_EQ(socket.respondersReceived, 1); + EXPECT_EQ(socket.respondersWithIdReceived, 1); + EXPECT_EQ(socket.respondersWithoutIdReceived, 0); } } // namespace test