From f6e222eb054420d329243f6066aefd54142e4b59 Mon Sep 17 00:00:00 2001
From: Andrey Obraztsov <anob@meta.com>
Date: Thu, 16 Jan 2025 21:30:37 -0800
Subject: [PATCH] Avoid a potential deadlock when using BridgeFromGoogleLogging

Summary:
Given that folly/logging could be used as LogSink in glog (ex - `folly::BridgeFromGoogleLogging`) there is a potential issue that could lead to a deadlock. An extreme example:

t0: LOG(FATAL) -> LogMessage::Flush() -> acquire Mutex -> BridgeFromGoogleLogging::send -> LogCategory::admitMessage -> std::abort() -> triggered signalHandler that waits for a thread t1

t1: LOG(INFO) -> LogMessage::Flush -> acquire Mutex -> deadlock

Reviewed By: andriigrynenko

Differential Revision: D68178188

fbshipit-source-id: 090bc848934663a8b1f8e96efa139dc442a99d60
---
 .../src/folly/logging/BridgeFromGoogleLogging.cpp      |  4 +++-
 third-party/folly/src/folly/logging/LogCategory.cpp    |  7 +++++--
 third-party/folly/src/folly/logging/LogCategory.h      | 10 +++++++++-
 .../folly/logging/test/BridgeFromGoogleLoggingTest.cpp |  7 +++++++
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/third-party/folly/src/folly/logging/BridgeFromGoogleLogging.cpp b/third-party/folly/src/folly/logging/BridgeFromGoogleLogging.cpp
index 370547e5cf204d..e5fbb77ef5f3b2 100644
--- a/third-party/folly/src/folly/logging/BridgeFromGoogleLogging.cpp
+++ b/third-party/folly/src/folly/logging/BridgeFromGoogleLogging.cpp
@@ -70,7 +70,9 @@ void BridgeFromGoogleLogging::send(
         static_cast<unsigned>(line),
         {},
         std::string{message, message_len}};
-    logger.getCategory()->admitMessage(logMessage);
+    // Make sure we don't abort on fatal messages and let glog library to
+    // handle it. As this call is done under lock, this could lead to a deadlock
+    logger.getCategory()->admitMessage(logMessage, /* skipAbortOnFatal */ true);
   }
 }
 
diff --git a/third-party/folly/src/folly/logging/LogCategory.cpp b/third-party/folly/src/folly/logging/LogCategory.cpp
index 371a856a43fc39..42c44ffe217769 100644
--- a/third-party/folly/src/folly/logging/LogCategory.cpp
+++ b/third-party/folly/src/folly/logging/LogCategory.cpp
@@ -47,7 +47,8 @@ LogCategory::LogCategory(StringPiece name, LogCategory* parent)
   parent_->firstChild_ = this;
 }
 
-void LogCategory::admitMessage(const LogMessage& message) const {
+void LogCategory::admitMessage(
+    const LogMessage& message, bool skipAbortOnFatal) const {
   processMessageWalker(this, message);
 
   // If this is a fatal message, flush the handlers to make sure the log
@@ -68,7 +69,9 @@ void LogCategory::admitMessage(const LogMessage& message) const {
           "\n");
       folly::writeFull(STDERR_FILENO, msg.data(), msg.size());
     }
-    std::abort();
+    if (!skipAbortOnFatal) {
+      std::abort();
+    }
   }
 }
 
diff --git a/third-party/folly/src/folly/logging/LogCategory.h b/third-party/folly/src/folly/logging/LogCategory.h
index 4298dbba3a603f..a864ebea976d1e 100644
--- a/third-party/folly/src/folly/logging/LogCategory.h
+++ b/third-party/folly/src/folly/logging/LogCategory.h
@@ -213,8 +213,16 @@ class LogCategory {
    *
    * This method generally should be invoked only through the logging macros,
    * rather than calling this directly.
+   *
+   * skipAbortOnFatal parameters provides more granular control on who
+   * is responsible for aborting process when calling the method directly.
+   * If skipAbortOnFatal is true, then this LogCategory will not trigger
+   * std::abort() if the LogMessage is fatal and the process should abort.
+   * This is necessary for the LoggerDB to handle fatal messages specially and
+   * only suitable for direct calls.
    */
-  void admitMessage(const LogMessage& message) const;
+  void admitMessage(
+      const LogMessage& message, bool skipAbortOnFatal = false) const;
 
   /**
    * Note: setLevelLocked() may only be called while holding the
diff --git a/third-party/folly/src/folly/logging/test/BridgeFromGoogleLoggingTest.cpp b/third-party/folly/src/folly/logging/test/BridgeFromGoogleLoggingTest.cpp
index 01600bb2e2977d..466dde3463a9ec 100644
--- a/third-party/folly/src/folly/logging/test/BridgeFromGoogleLoggingTest.cpp
+++ b/third-party/folly/src/folly/logging/test/BridgeFromGoogleLoggingTest.cpp
@@ -70,3 +70,10 @@ TEST_F(BridgeFromGoogleLoggingTest, bridge) {
   EXPECT_EQ(LogLevel::INFO, messages[0].first.getLevel());
   messages.clear();
 }
+
+TEST_F(BridgeFromGoogleLoggingTest, bridgeFatal) {
+  auto handler = make_shared<TestLogHandler>();
+  logging::BridgeFromGoogleLogging test_bridge{};
+  LoggerDB::get().getCategory("")->addHandler(handler);
+  ASSERT_DEATH([] { LOG(FATAL) << "test crash"; }(), "");
+}