Fix bug where returned caps would leak if the request was canceled.

This only happened if the Finish message indicating cancellation and the Return message happened to cross paths. If the Finish arrived before the Return was sent, then the server would send an empty response. If the Return was received before the Finish was sent, the client would properly record caps in the response. But if they crossed paths, the server would send back a full response with caps, but the client would never bother to inspect that response, thus leaking the caps. Fix is to set the flag to release all returned caps in the Finish message when cancelling.

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

@@ -558,6 +558,35 @@ TEST(Rpc, Release) {
   EXPECT_EQ(0, context.restorer.handleCount);
 }
 
+TEST(Rpc, ReleaseOnCancel) {
+  // At one time, there was a bug where if a Return contained capabilities, but the client had
+  // canceled the request and already send a Finish (which presumably didn't reach the server before
+  // the Return), then we'd leak those caps. Test for that.
+
+  TestContext context;
+
+  auto client = context.connect(test::TestSturdyRefObjectId::Tag::TEST_MORE_STUFF)
+      .castAs<test::TestMoreStuff>();
+  client.whenResolved().wait(context.waitScope);
+
+  {
+    auto promise = client.getHandleRequest().send();
+
+    // If the server receives cancellation too early, it won't even return a capability in the
+    // results, it will just return "canceled". We want to emulate the case where the return message
+    // and the cancel (finish) message cross paths. It turns out that exactly two evalLater()s get
+    // us there.
+    //
+    // TODO(cleanup): This is fragile, but I'm not sure how else to write it without a ton
+    //   of scaffolding.
+    kj::evalLater([]() {}).wait(context.waitScope);
+    kj::evalLater([]() {}).wait(context.waitScope);
+  }
+
+  for (uint i = 0; i < 16; i++) kj::evalLater([]() {}).wait(context.waitScope);
+  EXPECT_EQ(0, context.restorer.handleCount);
+}
+
 TEST(Rpc, TailCall) {
   TestContext context;
 

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

@@ -1157,21 +1157,26 @@ private:
 
     ~QuestionRef() {
       unwindDetector.catchExceptionsIfUnwinding([&]() {
+        auto& question = KJ_ASSERT_NONNULL(
+            connectionState->questions.find(id), "Question ID no longer on table?");
+
         // Send the "Finish" message (if the connection is not already broken).
         if (connectionState->connection.is<Connected>()) {
           auto message = connectionState->connection.get<Connected>()->newOutgoingMessage(
               messageSizeHint<rpc::Finish>());
           auto builder = message->getBody().getAs<rpc::Message>().initFinish();
           builder.setQuestionId(id);
-          builder.setReleaseResultCaps(false);
+          // If we're still awaiting a return, then this request is being canceled, and we're going
+          // to ignore any capabilities in the return message, so set releaseResultCaps true. If we
+          // already received the return, then we've already built local proxies for the caps and
+          // will send Release messages when those are destoryed.
+          builder.setReleaseResultCaps(question.isAwaitingReturn);
           message->send();
         }
 
         // Check if the question has returned and, if so, remove it from the table.
         // Remove question ID from the table.  Must do this *after* sending `Finish` to ensure that
         // the ID is not re-allocated before the `Finish` message can be sent.
-        auto& question = KJ_ASSERT_NONNULL(
-            connectionState->questions.find(id), "Question ID no longer on table?");
         if (question.isAwaitingReturn) {
           // Still waiting for return, so just remove the QuestionRef pointer from the table.
           question.selfRef = nullptr;
@@ -2173,7 +2178,8 @@ private:
           }
         }
 
-        // Looks like this question was canceled earlier, so `Finish` was already sent.  We can go
+        // Looks like this question was canceled earlier, so `Finish` was already sent, with
+        // `releaseResultCaps` set true so that we don't have to release them here.  We can go
         // ahead and delete it from the table.
         questions.erase(ret.getAnswerId(), *question);
       }