Page MenuHomePhabricator

Add FuzzerRemote
Needs ReviewPublic

Authored by aarongreen on Jan 12 2021, 9:45 AM.

Details

Summary

This changes defines the FuzzerRemoteInterface, and implements it for remote processes. When combined with an IPC library, this library can be used to fuzz remote processes.

This is change 14 of (at least) 20 for cross-process fuzzing support.

Diff Detail

Event Timeline

aarongreen created this revision.Jan 12 2021, 9:45 AM
aarongreen requested review of this revision.Jan 12 2021, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 9:45 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aarongreen updated this revision to Diff 321599.Feb 4 2021, 4:01 PM
aarongreen edited the summary of this revision. (Show Details)

Rebase

aarongreen updated this revision to Diff 321779.Feb 5 2021, 8:43 AM
morehouse added inline comments.Mar 9 2021, 11:26 AM
compiler-rt/lib/fuzzer/FuzzerRemote.cpp
58

Is there an implicit assumption that Counters and PCs have matching indices? Is it a problem if we have a sequence of calls like this:

  1. AddCoverage(Start1, Stop1)
  2. AddCoverage(Start2, Stop2)
  3. AddPCs(Beg2, End2)
  4. AddPCs(Beg1, End1)
241

None of these implementations actually use PID. Do we need it here?

compiler-rt/lib/fuzzer/FuzzerRemoteInterface.h
25

If fuzzer can call FuzzerRemote* and fuzzer_remote can call FuzzerProxy*, aren't the arrows backwards?

32

Please expand this warning to explain why we need C interfaces.

153

We don't actually use this constant.

compiler-rt/lib/fuzzer/tests/FuzzerRemoteUnittest.cpp
2
85–88

These are superfluous.

138

Just wondering: are the lambdas required to satisfy the compiler, or could we just directly assign: EF->__lsan_disable = FakeLSan::Disable?

260

This is not a "leak" from LSan point of view since the pointer to the memory is never lost.

278

This also is not a leak that LSan would report.

328

Also not a leak.

aarongreen added inline comments.Mar 11 2021, 9:30 AM
compiler-rt/lib/fuzzer/FuzzerRemote.cpp
58

I got a bit stuck thinking about this. I'm answering here first before finishing and uploading another diff.

I think you're right; there's nothing preventing two threads from calling dlopen simultaneously, which IIUC leads to the calls added by ModuleSanitizerCoverage::instrumentModule in llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp. That function *does* imply __sanitizer_cov_8bit_counters_init will be called before a corresponding call to __sanitizer_cov_pcs_init, but I think I might try to limit myself to an even weaker assumption: the order of calls to __sanitizer_cov_8bit_counters_init matches then order of calls to __sanitizer_cov_pcs_init for a particular thread, but the order of combined calls is unspecified.

The wrinkle is that the changes for module-relative features files for fork mode and merging *also* made this assumption about matching indices. I'll need to tweak TracePC with some thread_locals to handle that correctly. I assume I can't use the c++ stdlib at that point either, because platforms other than Linux and Fuchsia don't have a hermetic libclang_rt.fuzzer.a, i.e. they don't statically link against libc++.a. That will make any solution a little less aesthetically pleasing.

morehouse added inline comments.Mar 11 2021, 3:09 PM
compiler-rt/lib/fuzzer/FuzzerRemote.cpp
58

I think this assumption exists in current libFuzzer as well. For example, TracePC::UpdateObservedPCs relies on indices into Modules and ModulePCTable matching. Presumably dlopen in the presence of threads is rare enough that we haven't had issues in practice.

So this may not be something you need to fix.

The main thing that threw me off was the locking in these member functions, which implies multiple threads, which implies races outside the lock here.

Maybe a class comment explaining the multithread scenario you're trying to guard against with the mutex would be helpful.

aarongreen marked 12 inline comments as done.
aarongreen added inline comments.
compiler-rt/lib/fuzzer/FuzzerRemote.cpp
58

Okay, sounds good. I've tried to make concurrency clearer here with a comment describing the two possible competing threads: the module loader thread (__sanitizer_cov_*_init) and the IPC receiver thread (FuzzerRemote*). I'd already adjusted AddCounters/AddPCs to use thread_locals, so I've kept that.

As far as Mutex goes, I've tried to lock it less frequently: Now it gets locked by FinishExecution and stays locked until the next StartExecution, ensuring coverage is only added during a fuzzing iteration (which is when we expected new processes to be started). In a perfect world, there'd by an easy way to know when all modules that will ever be loaded are loaded and locking can be ignored, but this is as close as I think I can reasonably get.

241

Yes. As I attempted to describe in FuzzerRemoteInterface.h, both the |FuzzerProxy*| and |FuzzerRemote*| interfaces are implemented twice, once by libFuzzer[Remote], and once by the OS-specific IPC transport library. The transport lib needs the PID to know where to send the IPC, and the interface needs to match between the transport lib and FuzzerRemote. As a result, we have an extra parameter here that looks superfluous.

I've tried to add a comment that says much the same.

compiler-rt/lib/fuzzer/FuzzerRemoteInterface.h
32

I'm not sure what you want here. I've added that it's an external interface (to/from the OS-specific IPC transport library). Beyond that, I don't think you want a discussion about C++'s lack of a consistent ABI, or the benefits of being able to FFI from Rust or Golang...

Honestly, I just copy/pased from FuzzerInterface.h. If you'd like more, let me know.

153

Yet. Note the reference to it in the comment for FuzzerProxyAddCoverage. If that method fails, it will return this, as it does in D94523, the change that adds the implementation of FuzzerProxy. Based on the comment, I'd like to leave this here, even if it goes unused for a change or two.

compiler-rt/lib/fuzzer/tests/FuzzerRemoteUnittest.cpp
138

I think I originally captured this and had non-static methods. When that obviously didn't compile, I changed made them static, but neglected to drop the lambdas. Done.

260

Ah, one of those little comments that ends up opening a can of worms...

You're right, of course, so I tried introducing an explicit leak just to make sure it was caught... only to discover two issues:

  1. I was playing too fast and loose with the ExternalFunctions created by FuzzerRemotes constructor. That gets called from the __sanitizer_*_init functions, so I needed to remove any reliance on loadable modules. I can't quite wait until main to create EF, because I'm trying *very* hard not to require source-level changes in the remote process. This closest I can get is a late-prioritized global constructor, which seems to work.
  2. I hadn't tested with different combinations of sanitizer instrumentation and runtimes. The behavior varied a bit between -fsanitize=fuzzer-no-link, -fsanitize=address, both, and neither. I've faked more of LSan out in order to make this (and other) unit tests pass in any of those configurations.
278

Same.

Crud... I just noticed the .o files. New diff incoming...

aarongreen added inline comments.Mar 22 2021, 4:09 PM
compiler-rt/lib/fuzzer/FuzzerRemote.cpp
58

One other note on this thread: I apologize that the code moved around so much. As noted below, when looking at LSan I realized the initialization around here was dangerous and wrong. While on Fuchsia (and Linux now?) we statically link libc++, this code shouldn't assume that and shouldn't rely on std:: in the module constructor invoked callbacks. The result of that is the small list implementation above.

Harbormaster completed remote builds in B95118: Diff 332465.