Fix bug causing exception: "'Disembargo' of type 'senderLoopback' sent to an…

Fix bug causing exception: "'Disembargo' of type 'senderLoopback' sent to an object that does not point back to the sender."

The problem happened when pipelined calls were made on a promised capability, but then that capability turned out to be null. The promise resolving code incorrectly interpreted this as the remote promise having resolved to a local capability (because the "null" stub capability looks local), and so it would send a Disembargo message to flush the pipeline as required. However, the remote end would receive this Disembargo message and find it is addressed to a null capability, not a capability pointing back to the sender. This was treated as a protocol error, causing the receiver to close the connection.

The solution is to explicitly identity "null" capabilities so that we can distinguish this case. This change also has the benefit that now when you copy a null capability between messages with foo.setCap(bar.getCap()), the pointer will be set null in the destination, rather than becoming a reference to a local broken capability.

Thanks to David Renshaw for narrowing this down.

c++/src/capnp/arena.h

@@ -107,6 +107,7 @@ class BrokenCapFactory {
 
 public:
   virtual kj::Own<ClientHook> newBrokenCap(kj::StringPtr description) = 0;
+  virtual kj::Own<ClientHook> newNullCap() = 0;
 };
 #endif  // !CAPNP_LITE
 

c++/src/capnp/capability.c++

@@ -44,17 +44,20 @@ void setGlobalBrokenCapFactoryForLayoutCpp(BrokenCapFactory& factory);
 
 namespace {
 
+static kj::Own<ClientHook> newNullCap();
+
 class BrokenCapFactoryImpl: public _::BrokenCapFactory {
 public:
   kj::Own<ClientHook> newBrokenCap(kj::StringPtr description) override {
     return capnp::newBrokenCap(description);
   }
+  kj::Own<ClientHook> newNullCap() override {
+    return capnp::newNullCap();
+  }
 };
 
 static BrokenCapFactoryImpl brokenCapFactory;
 
-static kj::Own<ClientHook> newNullCap();
-
 }  // namespace
 
 ClientHook::ClientHook() {
@@ -102,6 +105,8 @@ kj::Promise<void> ClientHook::whenResolved() {
   }
 }
 
+const uint ClientHook::NULL_CAPABILITY_BRAND = 0;
+
 // =======================================================================================
 
 static inline uint firstSegmentSize(kj::Maybe<MessageSize> sizeHint) {
@@ -580,10 +585,11 @@ public:
 
 class BrokenClient final: public ClientHook, public kj::Refcounted {
 public:
-  BrokenClient(const kj::Exception& exception, bool resolved)
-      : exception(exception), resolved(resolved) {}
-  BrokenClient(const kj::StringPtr description, bool resolved)
-      : exception(kj::Exception::Type::FAILED, "", 0, kj::str(description)), resolved(resolved) {}
+  BrokenClient(const kj::Exception& exception, bool resolved, const void* brand = nullptr)
+      : exception(exception), resolved(resolved), brand(brand) {}
+  BrokenClient(const kj::StringPtr description, bool resolved, const void* brand = nullptr)
+      : exception(kj::Exception::Type::FAILED, "", 0, kj::str(description)),
+        resolved(resolved), brand(brand) {}
 
   Request<AnyPointer, AnyPointer> newCall(
       uint64_t interfaceId, uint16_t methodId, kj::Maybe<MessageSize> sizeHint) override {
@@ -612,12 +618,13 @@ public:
   }
 
   const void* getBrand() override {
-    return nullptr;
+    return brand;
   }
 
 private:
   kj::Exception exception;
   bool resolved;
+  const void* brand;
 };
 
 kj::Own<ClientHook> BrokenPipeline::getPipelinedCap(kj::ArrayPtr<const PipelineOp> ops) {
@@ -626,7 +633,8 @@ kj::Own<ClientHook> BrokenPipeline::getPipelinedCap(kj::ArrayPtr<const PipelineO
 
 kj::Own<ClientHook> newNullCap() {
   // A null capability, unlike other broken capabilities, is considered resolved.
-  return kj::refcounted<BrokenClient>("Called null capability.", true);
+  return kj::refcounted<BrokenClient>("Called null capability.", true,
+                                      &ClientHook::NULL_CAPABILITY_BRAND);
 }
 
 }  // namespace

c++/src/capnp/capability.h

@@ -551,6 +551,13 @@ public:
   // discover when a capability it needs to marshal is one that it created in the first place, and
   // therefore it can transfer the capability without proxying.
 
+  static const uint NULL_CAPABILITY_BRAND;
+  // Value is irrelevant; used for pointer.
+
+  inline bool isNull() { return getBrand() == &NULL_CAPABILITY_BRAND; }
+  // Returns true if the capability was created as a result of assigning a Client to null or by
+  // reading a null pointer out of a Cap'n Proto message.
+
   virtual void* getLocalServer(_::CapabilityServerSetBase& capServerSet);
   // If this is a local capability created through `capServerSet`, return the underlying Server.
   // Otherwise, return nullptr. Default implementation (which everyone except LocalClient should

c++/src/capnp/layout.c++

@@ -1561,7 +1561,11 @@ struct WireHelpers {
     if (!ref->isNull()) {
       zeroObject(segment, capTable, ref);
     }
-    ref->setCap(capTable->injectCap(kj::mv(cap)));
+    if (cap->isNull()) {
+      memset(ref, 0, sizeof(*ref));
+    } else {
+      ref->setCap(capTable->injectCap(kj::mv(cap)));
+    }
   }
 #endif  // !CAPNP_LITE
 
@@ -1895,7 +1899,7 @@ struct WireHelpers {
                "use the Cap'n Proto RPC system.");
 
     if (ref->isNull()) {
-      return brokenCapFactory->newBrokenCap("Calling null capability pointer.");
+      return brokenCapFactory->newNullCap();
     } else if (!ref->isCapability()) {
       KJ_FAIL_REQUIRE(
           "Message contains non-capability pointer where capability pointer was expected.") {

c++/src/capnp/rpc-test.c++

@@ -955,6 +955,34 @@ TEST(Rpc, EmbargoError) {
   getCallSequence(client, 1).wait(context.waitScope);
 }
 
+TEST(Rpc, EmbargoNull) {
+  // Set up a situation where we pipeline on a capability that ends up coming back null. This
+  // should NOT cause a Disembargo to be sent, but due to a bug in earlier versions of Cap'n Proto,
+  // a Disembargo was indeed sent to the null capability, which caused the server to disconnect
+  // due to protocol error.
+
+  TestContext context;
+
+  auto client = context.connect(test::TestSturdyRefObjectId::Tag::TEST_MORE_STUFF)
+      .castAs<test::TestMoreStuff>();
+
+  auto promise = client.getNullRequest().send();
+
+  auto cap = promise.getNullCap();
+
+  auto call0 = cap.getCallSequenceRequest().send();
+
+  promise.wait(context.waitScope);
+
+  auto call1 = cap.getCallSequenceRequest().send();
+
+  expectPromiseThrows(kj::mv(call0), context.waitScope);
+  expectPromiseThrows(kj::mv(call1), context.waitScope);
+
+  // Verify that we're still connected (there were no protocol errors).
+  getCallSequence(client, 0).wait(context.waitScope);
+}
+
 TEST(Rpc, CallBrokenPromise) {
   // Tell the server to call back to a promise client, then resolve the promise to an error.
 

c++/src/capnp/rpc.c++

@@ -886,8 +886,10 @@ private:
     bool receivedCall = false;
 
     void resolve(kj::Own<ClientHook> replacement, bool isError) {
-      if (replacement->getBrand() != connectionState.get() && receivedCall && !isError &&
-          connectionState->connection.is<Connected>()) {
+      const void* replacementBrand = replacement->getBrand();
+      if (replacementBrand != connectionState.get() &&
+          replacementBrand != &ClientHook::NULL_CAPABILITY_BRAND &&
+          receivedCall && !isError && connectionState->connection.is<Connected>()) {
         // The new capability is hosted locally, not on the remote machine.  And, we had made calls
         // to the promise.  We need to make sure those calls echo back to us before we allow new
         // calls to go directly to the local capability, so we need to set a local embargo and send

c++/src/capnp/test-util.c++

@@ -1112,6 +1112,10 @@ kj::Promise<void> TestMoreStuffImpl::getHandle(GetHandleContext context) {
   return kj::READY_NOW;
 }
 
+kj::Promise<void> TestMoreStuffImpl::getNull(GetNullContext context) {
+  return kj::READY_NOW;
+}
+
 #endif  // !CAPNP_LITE
 
 }  // namespace _ (private)

c++/src/capnp/test-util.h

@@ -284,6 +284,8 @@ public:
 
   kj::Promise<void> getHandle(GetHandleContext context) override;
 
+  kj::Promise<void> getNull(GetNullContext context) override;
+
 private:
   int& callCount;
   int& handleCount;

c++/src/capnp/test.capnp

@@ -801,6 +801,9 @@ interface TestMoreStuff extends(TestCallOrder) {
   getHandle @9 () -> (handle :TestHandle);
   # Get a new handle. Tests have an out-of-band way to check the current number of live handles, so
   # this can be used to test garbage collection.
+
+  getNull @10 () -> (nullCap :TestMoreStuff);
+  # Always returns a null capability.
 }
 
 interface TestMembrane {