Unsuccessful stab at fuzzing for the bug in D23362

[kcc]

Why is it unsuccessful?

lib/Fuzzer/CMakeLists.txt

7

also change the error message

tools/llvm-apint-fuzzer/llvm-apint-fuzzer.cpp

54

This function should always return 0.
Replace this with e.g.

if (Fst != Snd) abort();
return 0;

[jroelofs]

Unsuccessful because it let it run overnight, and it didn't find the UB in APInt::ashr (that was re-introduced in this patch).

The specific condition that the fuzzer would need to discover to trigger the bug would be to have a > 1bit -1, with a shift amount != the bitwidth.

lib/Fuzzer/CMakeLists.txt

7

Yeah, this was just a hacky one-off patch to play with the fuzzer, not something I was actually planning to commit upstream.

If you're ok with this particular part, I can put a real patch up to change this to "if asan or asan+ubsan", along with a more appropriate error message.

[kcc]

In D23838#524238, @jroelofs wrote:

Unsuccessful because it let it run overnight, and it didn't find the UB in APInt::ashr (that was re-introduced in this patch).

But can you reproduce the crash if you provide the crashy input?
I mean, if you know the bug (and actually have a fix) you have a way to reproduce it, don't you?

The specific condition that the fuzzer would need to discover to trigger the bug would be to have a > 1bit -1, with a shift amount != the bitwidth.

lib/Fuzzer/CMakeLists.txt

7

I would love to have your fuzzer and all related changes upstream.
As for this particular one:

• yours is one good way.
• another way is to allow optiona ubsan on top of asan, which will not require changing this file (AFAICT)

[kcc]

If you have the crashy input, you can feed it to the fuzzer binary in a natural way:

./fuzzer-binary crashy-file

[jroelofs]

In D23838#524379, @kcc wrote:

In D23838#524238, @jroelofs wrote:

Unsuccessful because it let it run overnight, and it didn't find the UB in APInt::ashr (that was re-introduced in this patch).

But can you reproduce the crash if you provide the crashy input?
I mean, if you know the bug (and actually have a fix) you have a way to reproduce it, don't you?

Humph. Good point. Thinking about it that way revealed a couple of bugs in my LLVMFuzzerTestOneInput implementation (I wasn't being careful enough about giving the fuzzer a chance to reproduce the same issue)... I'll post the fixes for that momentarily.

Even with the fixes though, I can't get the manual fuzz test to trip it. Also, I can no longer get the unit-test itself to trigger the UB either (which might explain quite a bit!). Not sure what's going on there :/

$ xxd crasher 
0000000: 0700 0000 ffff ffff ffff ffff 4000 0000  ............@...
$ ./bin/llvm-apint-fuzzer crasher 
INFO: Seed: 555293119
./bin/llvm-apint-fuzzer: Running 1 inputs 1 time(s) each.
Running: crasher
Executed crasher in 0 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***

[jroelofs]

lib/Fuzzer/CMakeLists.txt

7

another way is to allow optiona ubsan on top of asan, which will not require changing this file (AFAICT)

Not sure I understand this... what did you have in mind here?

[kcc]

Even with the fixes though, I can't get the manual fuzz test to trip it. Also, I can no longer get the unit-test itself to trigger the UB either (which might explain quite a bit!). Not sure what's going on there :/

Weird.
Can you trigger the bug if you revert to the buggy revision?

lib/Fuzzer/CMakeLists.txt

7

You don't have to allow asan *or ubsan in the cmake file.
Instead you may use asan *and* ubsan

lib/Fuzzer/CMakeLists.txt

 # Disable the coverage and sanitizer instrumentation for the fuzzer itself.
 set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -mpopcnt -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters -Werror")
 if( LLVM_USE_SANITIZE_COVERAGE )
-  if(NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address")
+  if((NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address") AND
+     (NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address;Undefined") AND
+     (NOT "${LLVM_USE_SANITIZER}" STREQUAL "Undefined;Address"))
     message(FATAL_ERROR
-      "LibFuzzer and its tests require LLVM_USE_SANITIZER=Address and "
+      "LibFuzzer and its tests require LLVM_USE_SANITIZER=Address "
+      "(or LLVM_USE_SANITIZER=Address;Undefined) and "
       "LLVM_USE_SANITIZE_COVERAGE=YES to be set."
       )
   endif()

lib/Support/APInt.cpp

   if (isSingleWord()) {
     if (shiftAmt == BitWidth)
       return APInt(BitWidth, 0); // undefined
-    return APInt(BitWidth, SignExtend64(VAL, BitWidth) >> shiftAmt);
+    else {
+      unsigned SignBit = APINT_BITS_PER_WORD - BitWidth;
+      return APInt(BitWidth,
+        (((int64_t(VAL) << SignBit) >> SignBit) >> shiftAmt));
+    }
   }
 
   // If all the bits were shifted out, the result is, technically, undefined.

tools/CMakeLists.txt

 add_llvm_tool_subdirectory(llvm-config)
 add_llvm_tool_subdirectory(llvm-lto)
 add_llvm_tool_subdirectory(llvm-profdata)
+add_llvm_tool_subdirectory(llvm-apint-fuzzer)
 
 # Projects supported via LLVM_EXTERNAL_*_SOURCE_DIR need to be explicitly
 # specified.

tools/llvm-apint-fuzzer/CMakeLists.txt

+if( LLVM_USE_SANITIZE_COVERAGE )
+  set(LLVM_LINK_COMPONENTS
+      Support
+      )
+  add_llvm_tool(llvm-apint-fuzzer 
+                llvm-apint-fuzzer.cpp)
+  target_link_libraries(llvm-apint-fuzzer
+                        LLVMFuzzer
+                        )
+endif()

tools/llvm-apint-fuzzer/llvm-apint-fuzzer.cpp

+//===--- fuzz-llvm-as.cpp - Fuzzer for llvm-as using lib/Fuzzer -----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <algorithm>
+#include <cstdint>
+
+using namespace llvm;
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
+  if (Size < 4)
+    return 0;
+  uint32_t ShiftAmount = llvm::support::endian::read32le(Data);
+  Data += 4;
+  Size -= 4;
+
+  if (Size < 8)
+    return 0;
+  uint64_t Value = llvm::support::endian::read64be(Data);
+  Data += 8;
+  Size -= 8;
+
+  if (Size < 1)
+    return 0;
+  uint32_t Bits = Data[0];
+  Data += 1;
+  Size -= 1;
+
+  // Limitation of APInt::ashr(): Bits can't be 0.
+  Bits = std::max(Bits, 1u);
+
+  // Limitation of APInt::ashr(): ShiftAmount can't be > Bits.
+  ShiftAmount = std::min(ShiftAmount, Bits);
+
+  APInt Input(Bits, Value, true);
+
+  // Check that:
+  //   (X >> ShiftAmount) >> 1
+  // can be commuted to:
+  //   (X >> 1) >> ShiftAmount
+  auto Fst = Input.ashr(ShiftAmount).ashr(1);
+  auto Snd = Input.ashr(1).ashr(ShiftAmount);
+
+  if (Fst != Snd)
+    abort();
+
+  // Sanity check: this should crash when ubsan'd, assuming the fix to APInt
+  // isn't applied:
+  const APInt neg_one(64, static_cast<uint64_t>(-1), true);
+  if (neg_one != neg_one.ashr(7))
+    abort();
+
+  return 0;
+}