Enable extra coverage counters on Windows

[MichaelSquires]

Summary

• Enable extra coverage counters on Windows.
• Update extra_counters.test to run on Windows also.
• Update TableLookupTest.cpp to include the required pragma/declspec for the extra coverage counters.

[MichaelSquires]

Formatted with clang-format

[MichaelSquires]

Formatted with clang-format

[rnk]

Seems reasonable to me, I have no familiarity with the extra counters feature, though. You might consider using explicit alignments.

Something else to consider is that the MSVC incremental linker adds padding between variables to allow resizing them between links. I think these days we just don't care about that anymore. In the past, we did work to make sure asan would work under those conditions.

compiler-rt/lib/fuzzer/FuzzerExtraCounters.cpp

46–48

Maybe __libfuzzer_extra_counter_a / __libfuzzer_extra_counter_z would make that clearer. "r, s, t" seems like a surprising part of the alphabet to use when establishing an ordering with lexicographic sorting.

89

Standard optimizations like loop idiom recognition will undo this. Is this enough to prevent ASan instrumentation? I'd suggest making these pointers volatile to be really confident that this won't become instrumented in the future.

[MichaelSquires]

Updates based on code review.

• Added explicit alignment to start and stop sections.
• Updated comments to better explain the alignment/padding requirements of the sections.
• Update the section names to be more intuitive with regards to sorting.
• Updated manual memset to use SecureZeroMemory to ensure it doesn't get optimized out.

[MichaelSquires]

Ping.

I made updates based on recommendations from @rnk.

I'm also realizing I likely don't have commit access and would appreciate if someone could commit on my behalf (assuming review approval).

Thanks!

[MichaelSquires]

Marking previous comments as done. I didn't realize this was something I could do - sorry, first time using this code review system.

[kcc]

One comment that I have is a request to limit the number of #ifdefs in the code to at most one.
We typically achieve this by having platform-specific code in a platform-specific file, guarded with an ifdef.

an extra ifdef in a test is probably unavoidable.

[MichaelSquires]

Moved platform specific code into separate files per request from @kcc.

[morehouse]

LGTM

[kcc]

compiler-rt/test/fuzzer/extra-counters.test

1

does this imply that the test will now work on Mac?

[MichaelSquires]

Mark extra counters test as unsupported on Darwin.

[MichaelSquires]

compiler-rt/test/fuzzer/extra-counters.test

1

No. Good catch, thanks! I updated the test to be unsupported on darwin.

[MichaelSquires]

Can someone commit this for me please? I don't have access.

[mstorsjo]

compiler-rt/lib/fuzzer/FuzzerExtraCountersWindows.cpp

15

This broke cross building from case sensitive filesystems with mingw; I pushed a fix for it in ab737d5367cdea30df71ef66f87fe2c6d88e7246.

compiler-rt/lib/fuzzer/CMakeLists.txt

 set(LIBFUZZER_SOURCES
   FuzzerCrossOver.cpp
   FuzzerDataFlowTrace.cpp
   FuzzerDriver.cpp
   FuzzerExtFunctionsDlsym.cpp
   FuzzerExtFunctionsWeak.cpp
   FuzzerExtFunctionsWindows.cpp
   FuzzerExtraCounters.cpp
+  FuzzerExtraCountersDarwin.cpp
+  FuzzerExtraCountersWindows.cpp
   FuzzerFork.cpp
   FuzzerIO.cpp
   FuzzerIOPosix.cpp
   FuzzerIOWindows.cpp
   FuzzerLoop.cpp
   FuzzerMerge.cpp
   FuzzerMutate.cpp
   FuzzerSHA1.cpp

compiler-rt/lib/fuzzer/FuzzerExtraCounters.cpp

void ClearExtraCounters() {  // hand-written memset, don't asan-ify.
   for (; Beg < End; Beg++) {
     *Beg = 0;
     __asm__ __volatile__("" : : : "memory");
   }
 }
 
 }  // namespace fuzzer
 
-#else
-// TODO: implement for other platforms.
-namespace fuzzer {
-uint8_t *ExtraCountersBegin() { return nullptr; }
-uint8_t *ExtraCountersEnd() { return nullptr; }
-void ClearExtraCounters() {}
-}  // namespace fuzzer
-
 #endif

compiler-rt/lib/fuzzer/FuzzerExtraCountersDarwin.cpp

+//===- FuzzerExtraCountersDarwin.cpp - Extra coverage counters for Darwin -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+// Extra coverage counters defined by user code for Darwin.
+//===----------------------------------------------------------------------===//
+
+#include "FuzzerPlatform.h"
+
+#if LIBFUZZER_APPLE
+
+namespace fuzzer {
+uint8_t *ExtraCountersBegin() { return nullptr; }
+uint8_t *ExtraCountersEnd() { return nullptr; }
+void ClearExtraCounters() {}
+} // namespace fuzzer
+
+#endif

compiler-rt/lib/fuzzer/FuzzerExtraCountersWindows.cpp

+//===- FuzzerExtraCountersWindows.cpp - Extra coverage counters for Win32 -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+// Extra coverage counters defined by user code for Windows.
+//===----------------------------------------------------------------------===//
+
+#include "FuzzerPlatform.h"
+#include <cstdint>
+
+#if LIBFUZZER_WINDOWS
+#include <Windows.h>
+
+namespace fuzzer {
+
+//
+// The __start___libfuzzer_extra_counters variable is align 16, size 16 to
+// ensure the padding between it and the next variable in this section (either
+// __libfuzzer_extra_counters or __stop___libfuzzer_extra_counters) will be
+// located at (__start___libfuzzer_extra_counters +
+// sizeof(__start___libfuzzer_extra_counters)). Otherwise, the calculation of
+// (stop - (start + sizeof(start))) might be skewed.
+//
+// The section name, __libfuzzer_extra_countaaa ends with "aaa", so it sorts
+// before __libfuzzer_extra_counters alphabetically. We want the start symbol to
+// be placed in the section just before the user supplied counters (if present).
+//
+#pragma section(".data$__libfuzzer_extra_countaaa")
+ATTRIBUTE_ALIGNED(16)
+__declspec(allocate(".data$__libfuzzer_extra_countaaa")) uint8_t
+    __start___libfuzzer_extra_counters[16] = {0};
+
+//
+// Example of what the user-supplied counters should look like. First, the
+// pragma to create the section name. It will fall alphabetically between
+// ".data$__libfuzzer_extra_countaaa" and ".data$__libfuzzer_extra_countzzz".
+// Next, the declspec to allocate the variable inside the specified section.
+// Finally, some array, struct, whatever that is used to track the counter data.
+// The size of this variable is computed at runtime by finding the difference of
+// __stop___libfuzzer_extra_counters and __start___libfuzzer_extra_counters +
+// sizeof(__start___libfuzzer_extra_counters).
+//
+
+//
+//     #pragma section(".data$__libfuzzer_extra_counters")
+//     __declspec(allocate(".data$__libfuzzer_extra_counters"))
+//         uint8_t any_name_variable[64 * 1024];
+//
+
+//
+// Here, the section name, __libfuzzer_extra_countzzz ends with "zzz", so it
+// sorts after __libfuzzer_extra_counters alphabetically. We want the stop
+// symbol to be placed in the section just after the user supplied counters (if
+// present). Align to 1 so there isn't any padding placed between this and the
+// previous variable.
+//
+#pragma section(".data$__libfuzzer_extra_countzzz")
+ATTRIBUTE_ALIGNED(1)
+__declspec(allocate(".data$__libfuzzer_extra_countzzz")) uint8_t
+    __stop___libfuzzer_extra_counters = 0;
+
+uint8_t *ExtraCountersBegin() {
+  return __start___libfuzzer_extra_counters +
+         sizeof(__start___libfuzzer_extra_counters);
+}
+
+uint8_t *ExtraCountersEnd() { return &__stop___libfuzzer_extra_counters; }
+
+ATTRIBUTE_NO_SANITIZE_ALL
+void ClearExtraCounters() {
+  uint8_t *Beg = ExtraCountersBegin();
+  SecureZeroMemory(Beg, ExtraCountersEnd() - Beg);
+}
+
+} // namespace fuzzer
+
+#endif

compiler-rt/test/fuzzer/TableLookupTest.cpp

 // The target code may declare more than one such array.
 //
 // Use either `Counters[Idx] = 1` or `Counters[Idx]++;`
 // depending on whether multiple occurrences of the event 'Idx'
 // is important to distinguish from one occurrence.
 #ifdef __linux__
 __attribute__((section("__libfuzzer_extra_counters")))
 #endif
+#ifdef _WIN32
+#  pragma section(".data$__libfuzzer_extra_counters")
+__declspec(allocate(".data$__libfuzzer_extra_counters"))
+#endif
     static uint8_t Counters[N];
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
   static std::set<uint16_t> SeenIdx;
   if (Size != 4) return 0;
   uint32_t Idx;
   memcpy(&Idx, Data, 4);
   Idx %= N;
   assert(Counters[Idx] == 0);  // libFuzzer should reset these between the runs.

compiler-rt/test/fuzzer/extra-counters.test

-REQUIRES: linux
+UNSUPPORTED: darwin
 
 RUN: %cpp_compiler %S/TableLookupTest.cpp -o %t-TableLookupTest
 RUN: not %run %t-TableLookupTest -print_final_stats=1 2>&1 | FileCheck %s
 CHECK: INFO: {{[0-9]+}} Extra Counters
 // Expecting >= 4096 new_units_added
 CHECK: stat::new_units_added:{{.*[4][0-9][0-9][0-9]}}