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

xplat/Flipper/FireAndForgetBasedFlipperResponder.h

@@ -7,9 +7,9 @@
 
 #pragma once
 
-#include "FlipperResponder.h"
-#include "FlipperConnectionManager.h"
 #include <folly/json.h>
+#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 =
+    const folly::dynamic message = idValid_
-        folly::dynamic::object("id", responseID_)("success", response);
+        ? 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 =
+    const folly::dynamic message = idValid_
-        folly::dynamic::object("id", responseID_)("error", response);
+        ? 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

xplat/Flipper/FlipperRSocketResponder.cpp

@@ -26,10 +26,13 @@ void FlipperRSocketResponder::handleFireAndForget(
   const auto payload = request.moveDataToString();
   std::unique_ptr<FireAndForgetBasedFlipperResponder> responder;
   auto message = folly::parseJson(payload);
-  if (message.find("id") != message.items().end()) {
+  auto idItr = message.find("id");
-    auto id = message["id"].getInt();
+  if (idItr == message.items().end()) {
     responder =
-        std::make_unique<FireAndForgetBasedFlipperResponder>(websocket_, id);
+        std::make_unique<FireAndForgetBasedFlipperResponder>(websocket_);
+  } else {
+    responder = std::make_unique<FireAndForgetBasedFlipperResponder>(
+        websocket_, idItr->second.getInt());
   }
 
   websocket_->onMessageReceived(

xplat/FlipperTestLib/FlipperConnectionManagerMock.h

@@ -7,6 +7,7 @@
 
 #pragma once
 
+#include <Flipper/FireAndForgetBasedFlipperResponder.h>
 #include <Flipper/FlipperConnectionManager.h>
 
 namespace facebook {
@@ -43,7 +44,17 @@ class FlipperConnectionManagerMock : public FlipperConnectionManager {
       const folly::dynamic& message,
       std::unique_ptr<FlipperResponder> responder) override {
     if (responder) {
-      respondersReceived++;
+      const FireAndForgetBasedFlipperResponder* const r =
+          dynamic_cast<FireAndForgetBasedFlipperResponder*>(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<folly::dynamic> messages;
   std::vector<folly::dynamic> messagesReceived;
-  int respondersReceived = 0;
+  int respondersWithIdReceived = 0;
+  int respondersWithoutIdReceived = 0;
 };
 
 } // namespace test

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