Skip to content

Commit

Permalink
MojoLPM: Add false positive filtering callback.
Browse files Browse the repository at this point in the history
In AddressSanitizer builds, we can use the error report callback to
detect whether a crash is occurring on the fuzzer thread and filter
these crashes to reduce the number of false positives.

Bug: 1372167
Change-Id: I63a025f93d4a63b3e614711d03e732e563d6daa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3952208
Reviewed-by: Chris Bookholt <bookholt@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Mark Brand <markbrand@google.com>
Cr-Commit-Position: refs/heads/main@{#1084873}
  • Loading branch information
c01db33f authored and Chromium LUCI CQ committed Dec 19, 2022
1 parent 46d0916 commit b96911d
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 3 deletions.
32 changes: 32 additions & 0 deletions content/test/fuzzer/mojolpm_fuzzer_support.cc
Expand Up @@ -5,14 +5,41 @@
#include "content/test/fuzzer/mojolpm_fuzzer_support.h"

#include "base/command_line.h"
#include "base/debug/asan_service.h"
#include "base/i18n/icu_util.h"
#include "base/test/test_timeouts.h"
#include "base/threading/platform_thread.h"
#include "content/browser/network_service_instance_impl.h" // [nogncheck]
#include "content/browser/storage_partition_impl.h" // [nogncheck]
#include "content/browser/storage_partition_impl_map.h" // [nogncheck]

namespace content {
namespace mojolpm {

#if defined(ADDRESS_SANITIZER)
static void FalsePositiveErrorReportCallback(const char* reason,
bool* should_exit_cleanly) {
if (!strcmp(base::PlatformThread::GetName(), "fuzzer_thread")) {
base::debug::AsanService::GetInstance()->Log(
"MojoLPM: FALSE POSITIVE\n"
"This crash occurred on the fuzzer thread, so it is a false positive "
"and "
"\ndoes not represent a security issue. In MojoLPM, the fuzzer thread "
"\nrepresents the unprivileged renderer process.\n");
*should_exit_cleanly = true;
}
}

static void AddFalsePositiveErrorReportCallback() {
static bool registered = false;
if (!registered) {
base::debug::AsanService::GetInstance()->AddErrorCallback(
FalsePositiveErrorReportCallback);
registered = true;
}
}
#endif // defined(ADDRESS_SANITIZER)

FuzzerEnvironment::FuzzerEnvironment(int argc, const char* const* argv)
: command_line_initialized_(base::CommandLine::Init(argc, argv)),
fuzzer_thread_("fuzzer_thread") {
Expand All @@ -26,6 +53,11 @@ FuzzerEnvironment::FuzzerEnvironment(int argc, const char* const* argv)
StoragePartitionImpl::ForceInProcessStorageServiceForTesting();

fuzzer_thread_.StartAndWaitForTesting();

#if defined(ADDRESS_SANITIZER)
base::debug::AsanService::GetInstance()->Initialize();
AddFalsePositiveErrorReportCallback();
#endif // defined(ADDRESS_SANITIZER)
}

FuzzerEnvironment::~FuzzerEnvironment() {}
Expand Down
21 changes: 18 additions & 3 deletions mojo/docs/mojolpm.md
Expand Up @@ -260,11 +260,11 @@ Mojo Core, and loading ICU datafiles.
A key difference between our needs here and those of a normal unittest is that
we very likely do not want to be running in a special single-threaded mode. We
want to be able to trigger issues related to threading, sequencing and ordering,
and making sure that the UI, IO and threadpool threads behave as close to a
and making sure that the UI, IO and threadpool threads behave as close to a
normal browser process as possible is desirable.
It's likely better to be conservative here - while it might appear that an
interface to be tested has no interaction with the UI thread, and so we could
It's likely better to be conservative here - while it might appear that an
interface to be tested has no interaction with the UI thread, and so we could
save some resources by only having a real IO thread, it's often very difficult
to establish this with certainty.
Expand Down Expand Up @@ -437,6 +437,7 @@ the rough structure of the presentation service fuzzer")
Make a corpus directory and fire up your shiny new fuzzer!

```
~/chromium/src% set ASAN_OPTIONS=detect_odr_violation=0,handle_abort=1,handle_sigtrap=1,handle_sigill=1
~/chromium/src% out/Default/code_cache_host_mojolpm_fuzzer /dev/shm/corpus
INFO: Seed: 3273881842
INFO: Loaded 1 modules (1121912 inline 8-bit counters): 1121912 [0x559151a1aea8, 0x559151b2cd20),
Expand Down Expand Up @@ -720,6 +721,20 @@ implementation should never, under any circumstances be running on this thread,
so any crash on this thread is the result of a bug in the fuzzer itself, or
one of the other causes mentioned below.

In AddressSanitizer builds this case can be automatically identified by
additional instrumentation, which is implemented as part of
`content::mojolpm::FuzzerEnvironment` but will need to be duplicated for fuzzers
in other areas of the codebase. This instrumentation prints additional output as
part of the ASan report, and should make the fuzzer exit cleanly for these false
positives so that further instrumentation should ignore these crashes.

```
MojoLPM: FALSE POSITIVE
This crash occurred on the fuzzer thread, so it is a false positive and
does not represent a security issue. In MojoLPM, the fuzzer thread
represents the unprivileged renderer process.
```

The second is DCHECK or other failures during Mojo serialization. Various traits
assert that they are serializing reasonable values - since we need to reuse this
serialization code in the fuzzer to produce input to the implementation, we can
Expand Down

0 comments on commit b96911d

Please sign in to comment.