From ccd8afabde5a21071f9066b0acbf61740fa7b6c8 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 22 May 2023 17:02:17 -0700 Subject: [PATCH] Make FML_LOG safe from static initialization (flutter/engine#42219) I ran into this while trying to get some printing going for places where we're creating thread local keys. Supposedly, just including `` should statically initialize `std::cout/cerr`, but it gets really hard to reason about whether your statically initialized code is going to be initialized before or after that happens. I tried making sure that the TU for `fml/logging.cc` did that initialization statically, but that also failed in the verison of the test included here (it passed in some other iterations that modified run_all_unittests.cc). We _could_ make sure it happens each and every time we touch `std::cerr` but ... we could also just use `fprintf(stderr, ...)` and it works just fine. /cc @flar who ran into problems around this a little while back and was asking about it. --- engine/src/flutter/fml/logging.cc | 5 +-- engine/src/flutter/fml/logging_unittests.cc | 37 +++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/engine/src/flutter/fml/logging.cc b/engine/src/flutter/fml/logging.cc index 5a0280e0f0..61b4b5cf27 100644 --- a/engine/src/flutter/fml/logging.cc +++ b/engine/src/flutter/fml/logging.cc @@ -123,8 +123,9 @@ LogMessage::~LogMessage() { fx_logger_log_with_source(fx_log_get_logger(), fx_severity, nullptr, file_, line_, stream_.str().c_str()); #else - std::cerr << stream_.str(); - std::cerr.flush(); + // Don't use std::cerr here, because it may not be initialized properly yet. + fprintf(stderr, "%s", stream_.str().c_str()); + fflush(stderr); #endif if (severity_ >= LOG_FATAL) { diff --git a/engine/src/flutter/fml/logging_unittests.cc b/engine/src/flutter/fml/logging_unittests.cc index a64d33e52f..b418397640 100644 --- a/engine/src/flutter/fml/logging_unittests.cc +++ b/engine/src/flutter/fml/logging_unittests.cc @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include + #include "flutter/fml/build_config.h" #include "flutter/fml/log_settings.h" #include "flutter/fml/logging.h" @@ -19,6 +21,41 @@ namespace fml { namespace testing { +#ifndef OS_FUCHSIA +class MakeSureFmlLogDoesNotSegfaultWhenStaticallyCalled { + public: + MakeSureFmlLogDoesNotSegfaultWhenStaticallyCalled() { + SegfaultCatcher catcher; + // If this line causes a segfault, FML is using a method of logging that is + // not safe from static initialization on your platform. + FML_LOG(INFO) + << "This log exists to verify that static logging from FML works."; + } + + private: + struct SegfaultCatcher { + typedef void (*sighandler_t)(int); + + SegfaultCatcher() { + handler = ::signal(SIGSEGV, SegfaultHandler); + FML_CHECK(handler != SIG_ERR); + } + + ~SegfaultCatcher() { FML_CHECK(::signal(SIGSEGV, handler) != SIG_ERR); } + + static void SegfaultHandler(int signal) { + fprintf(stderr, + "FML failed to handle logging from static initialization.\n"); + exit(signal); + } + + sighandler_t handler; + }; +}; + +static MakeSureFmlLogDoesNotSegfaultWhenStaticallyCalled fml_log_static_check_; +#endif // !defined(OS_FUCHSIA) + int UnreachableScopeWithoutReturnDoesNotMakeCompilerMad() { KillProcess(); // return 0; <--- Missing but compiler is fine.