From 1273ae3546725b150f57973d2eaaf3bb61d7fcd9 Mon Sep 17 00:00:00 2001
From: Mingtao Yang <mingtao@meta.com>
Date: Sat, 7 Dec 2024 05:42:35 -0800
Subject: [PATCH] Ensure handleError doesn't throw

Summary:
If `handleError` is invoked with a supplied `AlertDescription`, it will attempt
to use the current write record layer to encode an alert to be sent to the peer.

The write record layer `write()` may throw. This is problematic since
`handleError` can be invoked itself in the catch handler (e.g. during
`processSocketData`). Uncaught exceptions will then propagate out of the state
machine into the I/O layer, which may be invoked under a noexcept context.

This diff catches exceptions that may be thrown by the write record layer during
`handleError`, skipping writing the alert if one is thrown.

Reviewed By: knekritz

Differential Revision: D66526269

fbshipit-source-id: 2e0ebb91767d3543d5f0aa10eaeff3e38ba821a2
---
 fizz/client/ClientProtocol.cpp          | 21 ++++++++++++++-------
 fizz/client/test/ClientProtocolTest.cpp | 14 ++++++++++++++
 fizz/server/ServerProtocol.cpp          | 21 ++++++++++++++-------
 fizz/server/test/ServerProtocolTest.cpp | 15 +++++++++++++++
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/fizz/client/ClientProtocol.cpp b/fizz/client/ClientProtocol.cpp
index 4fd11e977c3..df7669ae1ec 100644
--- a/fizz/client/ClientProtocol.cpp
+++ b/fizz/client/ClientProtocol.cpp
@@ -286,15 +286,22 @@ Actions handleError(
     newState.writeRecordLayer() = nullptr;
     newState.readRecordLayer() = nullptr;
   });
+
+  Actions actions;
+  actions.emplace_back(std::move(transition));
+
   if (alertDesc && state.writeRecordLayer()) {
-    Alert alert(*alertDesc);
-    WriteToSocket write;
-    write.contents.emplace_back(
-        state.writeRecordLayer()->writeAlert(std::move(alert)));
-    return actions(std::move(transition), std::move(write), std::move(error));
-  } else {
-    return actions(std::move(transition), std::move(error));
+    try {
+      Alert alert(*alertDesc);
+      WriteToSocket write;
+      write.contents.emplace_back(
+          state.writeRecordLayer()->writeAlert(std::move(alert)));
+      actions.emplace_back(std::move(write));
+    } catch (...) {
+    }
   }
+  actions.emplace_back(std::move(error));
+  return actions;
 }
 
 Actions handleAppCloseImmediate(const State& state) {
diff --git a/fizz/client/test/ClientProtocolTest.cpp b/fizz/client/test/ClientProtocolTest.cpp
index d9baf56ac91..012f6b0e89a 100644
--- a/fizz/client/test/ClientProtocolTest.cpp
+++ b/fizz/client/test/ClientProtocolTest.cpp
@@ -5918,6 +5918,20 @@ TEST_F(ClientProtocolTest, TestPskWithoutCerts) {
   EXPECT_EQ(state_.clientAuthRequested().value(), ClientAuthType::NotRequested);
 }
 
+TEST_F(ClientProtocolTest, HandleErrorDoesntThrow) {
+  setupAcceptingData();
+  EXPECT_CALL(*mockWrite_, _write(_, _))
+      .WillOnce(Throw(std::runtime_error("write error")));
+
+  ReportError error("some error");
+  AlertDescription ad = AlertDescription::internal_error;
+
+  auto shouldNotThrow = [&] {
+    auto actions = detail::handleError(state_, std::move(error), ad);
+    expectActions<MutateState, ReportError>(actions);
+  };
+  EXPECT_NO_THROW(shouldNotThrow());
+}
 } // namespace test
 } // namespace client
 } // namespace fizz
diff --git a/fizz/server/ServerProtocol.cpp b/fizz/server/ServerProtocol.cpp
index 66e0600886b..ccf12172426 100644
--- a/fizz/server/ServerProtocol.cpp
+++ b/fizz/server/ServerProtocol.cpp
@@ -286,15 +286,22 @@ Actions handleError(
     newState.writeRecordLayer() = nullptr;
     newState.readRecordLayer() = nullptr;
   });
+
+  Actions actions;
+  actions.emplace_back(std::move(transition));
+
   if (alertDesc && state.writeRecordLayer()) {
-    Alert alert(*alertDesc);
-    WriteToSocket write;
-    write.contents.emplace_back(
-        state.writeRecordLayer()->writeAlert(std::move(alert)));
-    return actions(std::move(transition), std::move(write), std::move(error));
-  } else {
-    return actions(std::move(transition), std::move(error));
+    try {
+      Alert alert(*alertDesc);
+      WriteToSocket write;
+      write.contents.emplace_back(
+          state.writeRecordLayer()->writeAlert(std::move(alert)));
+      actions.emplace_back(std::move(write));
+    } catch (...) {
+    }
   }
+  actions.emplace_back(std::move(error));
+  return actions;
 }
 
 Actions handleAppCloseImmediate(const State& state) {
diff --git a/fizz/server/test/ServerProtocolTest.cpp b/fizz/server/test/ServerProtocolTest.cpp
index cb3474f5c2b..1d0fb9ce660 100644
--- a/fizz/server/test/ServerProtocolTest.cpp
+++ b/fizz/server/test/ServerProtocolTest.cpp
@@ -6743,6 +6743,21 @@ TEST_F(ServerProtocolTest, AsyncKeyExchangeTest) {
   EXPECT_TRUE(
       std::all_of(begin(cr), end(cr), [](auto c) { return c == 0x44; }));
 }
+
+TEST_F(ServerProtocolTest, HandleErrorDoesntThrow) {
+  setUpAcceptingData();
+  EXPECT_CALL(*appWrite_, _write(_, _))
+      .WillOnce(Throw(std::runtime_error("write error")));
+
+  ReportError error("some error");
+  AlertDescription ad = AlertDescription::internal_error;
+
+  auto shouldNotThrow = [&] {
+    auto actions = detail::handleError(state_, std::move(error), ad);
+    expectActions<MutateState, ReportError>(actions);
+  };
+  EXPECT_NO_THROW(shouldNotThrow());
+}
 } // namespace test
 } // namespace server
 } // namespace fizz