Separate check failure message parts more clearly.
This CL encompasses two changes:
1. Always end check failure messages with a period.
2. In fuzzing mode, replace the period with a semicolon.
For example:
CHECK(false);
Yields:
Check failed: false.
Or, with additional data streamed in:
CHECK(false) << "foo";
The output is:
Check failed: false. foo
In fuzzing mode, however, in a bid to help machines understand where
to draw the line between the compile-time static part ("false") and
the runtime-variable part ("foo"), we use a semicolon instead. It
seems quite difficult to manage to embed a semilicon in the
compile-time static part. Thus the examples above become:
Check failed: false;
Check failed: false; foo
Change-Id: Ia09851382f0d3207d0720d204cef26e223ade024
Bug: 443588668
Cq-Include-Trybots: luci.chromium.try:chromeos-x64-libfuzzer-asan-rel-tests,linux-x64-libfuzzer-asan-dbg-tests,linux-x64-libfuzzer-asan-rel-tests,linux-x64-libfuzzer-msan-rel-tests,linux-x64-libfuzzer-ubsan-rel-tests,linux-x86-libfuzzer-asan-rel-tests,mac-arm64-libfuzzer-asan-rel-tests,win-x64-libfuzzer-asan-rel-tests,linux-x64-centipede-asan-rel-tests
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6917708
Reviewed-by: Antonio Alphonse <alphonsea@google.com>
Commit-Queue: Antonio Alphonse <alphonsea@google.com>
Auto-Submit: Titouan Rigoudy <titouan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1558298}
diff --git a/base/check.cc b/base/check.cc
index 8c274fef..d69a6aa 100644
--- a/base/check.cc
+++ b/base/check.cc
@@ -9,17 +9,51 @@
#include "base/check_op.h"
#include "base/check_version_internal.h"
#include "base/debug/alias.h"
+#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
#include "base/logging.h"
#include "base/thread_annotations.h"
#include "base/types/cxx23_to_underlying.h"
#include "build/build_config.h"
-#include "base/debug/crash_logging.h"
namespace logging {
namespace {
+// End check failure messages with a period, e.g.:
+//
+// CHECK(false);
+//
+// Yields:
+//
+// Check failed: false.
+//
+// Or, with additional data streamed in:
+//
+// CHECK(false) << "foo";
+//
+// The output is:
+//
+// Check failed: false. foo
+//
+// In fuzzing mode, however, in a bid to help machines understand where to draw
+// the line between the compile-time static part ("false") and the
+// runtime-variable part ("foo"), we use a semicolon instead. It seems quite
+// difficult to manage to embed a semilicon in the compile-time static part.
+// Thus the examples above become:
+//
+// Check failed: false;
+// Check failed: false; foo
+//
+// See crbug.com/443588668 for more details.
+constexpr char kCheckFailureMessageSeparator[] =
+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
+ "; "
+#else
+ ". "
+#endif
+ ;
+
LogSeverity GetDumpSeverity() {
#if defined(OFFICIAL_BUILD)
return DCHECK_IS_ON() ? LOGGING_DCHECK : LOGGING_ERROR;
@@ -217,7 +251,8 @@
auto* const log_message = new CheckLogMessage(
location, GetCheckSeverity(fatal_milestone), fatal_milestone);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << condition << ". ";
+ log_message->stream() << "Check failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckError(log_message);
}
@@ -227,7 +262,8 @@
auto* const log_message = new CheckLogMessage(
location, GetCheckSeverity(fatal_milestone), fatal_milestone);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << log_message_str;
+ log_message->stream() << "Check failed: " << log_message_str
+ << kCheckFailureMessageSeparator;
free(log_message_str);
return log_message;
}
@@ -235,14 +271,16 @@
CheckError CheckError::DCheck(const char* condition,
const base::Location& location) {
auto* const log_message = new DCheckLogMessage(location);
- log_message->stream() << "DCHECK failed: " << condition << ". ";
+ log_message->stream() << "DCHECK failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckError(log_message);
}
LogMessage* CheckError::DCheckOp(char* log_message_str,
const base::Location& location) {
auto* const log_message = new DCheckLogMessage(location);
- log_message->stream() << "DCHECK failed: " << log_message_str;
+ log_message->stream() << "DCHECK failed: " << log_message_str
+ << kCheckFailureMessageSeparator;
free(log_message_str);
return log_message;
}
@@ -253,7 +291,8 @@
new CheckLogMessage(location, GetDumpSeverity(),
base::NotFatalUntil::NoSpecifiedMilestoneInternal);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << condition << ". ";
+ log_message->stream() << "Check failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckError(log_message);
}
@@ -263,7 +302,8 @@
new CheckLogMessage(location, GetDumpSeverity(),
base::NotFatalUntil::NoSpecifiedMilestoneInternal);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << log_message_str;
+ log_message->stream() << "Check failed: " << log_message_str
+ << kCheckFailureMessageSeparator;
free(log_message_str);
return log_message;
}
@@ -276,7 +316,8 @@
#elif BUILDFLAG(IS_POSIX) || BUILDFLAG(IS_FUCHSIA)
auto* const log_message = new DCheckErrnoLogMessage(location, err_code);
#endif
- log_message->stream() << "DCHECK failed: " << condition << ". ";
+ log_message->stream() << "DCHECK failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckError(log_message);
}
@@ -285,7 +326,8 @@
auto* const log_message = new LogMessage(
location.file_name(), location.line_number(), LOGGING_ERROR);
// TODO(pbos): Make this output NOTIMPLEMENTED instead of Not implemented.
- log_message->stream() << "Not implemented reached in " << function;
+ log_message->stream() << "Not implemented reached in " << function
+ << kCheckFailureMessageSeparator;
return CheckError(log_message);
}
@@ -332,7 +374,8 @@
new CheckLogMessage(location, LOGGING_FATAL,
base::NotFatalUntil::NoSpecifiedMilestoneInternal);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << condition << ". ";
+ log_message->stream() << "Check failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckNoreturnError(log_message);
}
@@ -342,7 +385,8 @@
new CheckLogMessage(location, LOGGING_FATAL,
base::NotFatalUntil::NoSpecifiedMilestoneInternal);
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << log_message_str;
+ log_message->stream() << "Check failed: " << log_message_str
+ << kCheckFailureMessageSeparator;
free(log_message_str);
return log_message;
}
@@ -358,7 +402,8 @@
location.file_name(), location.line_number(), LOGGING_FATAL, err_code);
#endif
// TODO(pbos): Make this output CHECK instead of Check.
- log_message->stream() << "Check failed: " << condition << ". ";
+ log_message->stream() << "Check failed: " << condition
+ << kCheckFailureMessageSeparator;
return CheckNoreturnError(log_message);
}
diff --git a/base/check_unittest.cc b/base/check_unittest.cc
index ae5e0d0..06ca33e 100644
--- a/base/check_unittest.cc
+++ b/base/check_unittest.cc
@@ -177,9 +177,9 @@
EXPECT_CHECK("Check failed: false. foo", CHECK(false) << "foo");
double a = 2, b = 1;
- EXPECT_CHECK("Check failed: a < b (2.000000 vs. 1.000000)", CHECK_LT(a, b));
+ EXPECT_CHECK("Check failed: a < b (2.000000 vs. 1.000000). ", CHECK_LT(a, b));
- EXPECT_CHECK("Check failed: a < b (2.000000 vs. 1.000000)custom message",
+ EXPECT_CHECK("Check failed: a < b (2.000000 vs. 1.000000). custom message",
CHECK_LT(a, b) << "custom message");
}
@@ -189,29 +189,17 @@
std::string err =
logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode());
- EXPECT_CHECK(
- "Check failed: fopen(file, \"r\") != nullptr."
- " : " +
- err,
- PCHECK(fopen(file, "r") != nullptr));
+ EXPECT_CHECK("Check failed: fopen(file, \"r\") != nullptr. : " + err,
+ PCHECK(fopen(file, "r") != nullptr));
- EXPECT_CHECK(
- "Check failed: fopen(file, \"r\") != nullptr."
- " foo: " +
- err,
- PCHECK(fopen(file, "r") != nullptr) << "foo");
+ EXPECT_CHECK("Check failed: fopen(file, \"r\") != nullptr. foo: " + err,
+ PCHECK(fopen(file, "r") != nullptr) << "foo");
- EXPECT_DCHECK(
- "DCHECK failed: fopen(file, \"r\") != nullptr."
- " : " +
- err,
- DPCHECK(fopen(file, "r") != nullptr));
+ EXPECT_DCHECK("DCHECK failed: fopen(file, \"r\") != nullptr. : " + err,
+ DPCHECK(fopen(file, "r") != nullptr));
- EXPECT_DCHECK(
- "DCHECK failed: fopen(file, \"r\") != nullptr."
- " foo: " +
- err,
- DPCHECK(fopen(file, "r") != nullptr) << "foo");
+ EXPECT_DCHECK("DCHECK failed: fopen(file, \"r\") != nullptr. foo: " + err,
+ DPCHECK(fopen(file, "r") != nullptr) << "foo");
}
TEST(CheckDeathTest, CheckOp) {
@@ -619,7 +607,7 @@
EXPECT_LOG_ERROR_WITH_FILENAME(base::Location::Current().file_name(),
base::Location::Current().line_number(),
NOTIMPLEMENTED() << "foo",
- expected_msg + "foo\n");
+ expected_msg + ". foo\n");
#else
// Expect nothing.
EXPECT_NO_LOG(NOTIMPLEMENTED() << "foo");
@@ -632,7 +620,7 @@
TEST(CheckTest, NotImplementedLogOnce) {
static const std::string expected_msg =
- kNotImplementedMessage + "void (anonymous namespace)::NiLogOnce()\n";
+ kNotImplementedMessage + "void (anonymous namespace)::NiLogOnce(). \n";
#if DCHECK_IS_ON()
EXPECT_LOG_ERROR_WITH_FILENAME(base::Location::Current().file_name(),
@@ -654,16 +642,17 @@
TEST(CheckTest, NotImplementedLogOnceWithStreamedParams) {
static const std::string expected_msg1 =
kNotImplementedMessage +
- "void (anonymous namespace)::NiLogTenTimesWithStream() iteration: 0\n";
+ "void (anonymous namespace)::NiLogTenTimesWithStream(). "
+ " iteration: 0\n";
#if DCHECK_IS_ON()
// Expect LOG(ERROR) with streamed params intact, exactly once.
EXPECT_LOG_ERROR_WITH_FILENAME(base::Location::Current().file_name(),
- base::Location::Current().line_number() - 13,
+ base::Location::Current().line_number() - 14,
NiLogTenTimesWithStream(), expected_msg1);
// A different NOTIMPLEMENTED_LOG_ONCE() call is still logged.
static const std::string expected_msg2 =
- kNotImplementedMessage + __PRETTY_FUNCTION__ + "tree fish\n";
+ kNotImplementedMessage + __PRETTY_FUNCTION__ + ". tree fish\n";
EXPECT_LOG_ERROR_WITH_FILENAME(base::Location::Current().file_name(),
base::Location::Current().line_number(),
NOTIMPLEMENTED_LOG_ONCE() << "tree fish",
