GH-45180: [C++][Fuzzing] Fix Negation bug discovered by fuzzing

[zeroshade]

Rationale for this change

If the value for Decimal32 or Decimal64 is INT32_MIN or INT64_MIN respectively, then UBSAN reports an issue when calling Negate on them due to overflow.

What changes are included in this PR?

Have the Negate methods of Decimal32 and Decimal64 use arrow::internal::SafeSignedNegate.

Are these changes tested?

Unit tests were added for both cases which were able to reproduce the problem when UBSAN was on without the fix.

Are there any user-facing changes?

No.

• OSS-Fuzz issue: https://issues.oss-fuzz.com/issues/371239168

• GitHub Issue: [C++][Fuzzing] Fix bug discovered by fuzzing #45180

[github-actions]

⚠️ GitHub issue #45180 has been automatically assigned in GitHub to PR creator.

[pitrou]

Can you post the original backtrace? IMHO, Decimal32 and Decimal64 are not supposed to hold a number that large in the first place.

[zeroshade]

@pitrou I agree, they aren't supposed to hold values that large. Which is why this was found during fuzzing rather than regular testing I suppose.

The original stacktrace shows that the issue was encountered via the Abs() call through FitsInPrecision:

[Environment] UBSAN_OPTIONS=exitcode=77:print_stacktrace=1:silence_unsigned_overflow=1
+----------------------------------------Release Build Stacktrace----------------------------------------+
Command: /mnt/scratch0/clusterfuzz/resources/platform/linux/unshare -c -n /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_arrow_54ac8e3130fd5b639a9b3df39d9203298c5a0908/revisions/arrow-ipc-file-fuzz -rss_limit_mb=2560 -timeout=60 -runs=100 /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/crash-367fa1959877658ff787a606c40c2cbb09eb0888
Time ran: 0.6160283088684082
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1259504269
INFO: Loaded 1 modules   (825716 inline 8-bit counters): 825716 [0x5be0a55a46c0, 0x5be0a566e034),
INFO: Loaded 1 PC tables (825716 PCs): 825716 [0x5be0a566e038,0x5be0a6307778),
/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_arrow_54ac8e3130fd5b639a9b3df39d9203298c5a0908/revisions/arrow-ipc-file-fuzz: Running 1 inputs 100 time(s) each.
Running: /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/crash-367fa1959877658ff787a606c40c2cbb09eb0888
/src/arrow/cpp/src/arrow/util/basic_decimal.h:280:14: runtime error: negation of -2147483648 cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
    #0 0x5be0a411eac1 in Negate [arrow/cpp/src/arrow/util/basic_decimal.h:280](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/basic_decimal.h#L280):14
    #1 0x5be0a411eac1 in Abs [arrow/cpp/src/arrow/util/basic_decimal.h:285](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/basic_decimal.h#L285):46
    #2 0x5be0a411eac1 in Abs [arrow/cpp/src/arrow/util/basic_decimal.h:290](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/basic_decimal.h#L290):19
    #3 0x5be0a411eac1 in arrow::BasicDecimal32::FitsInPrecision(int) const [arrow/cpp/src/arrow/util/basic_decimal.cc:256](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/basic_decimal.cc#L256):10
    #4 0x5be0a3a714b2 in operator() [arrow/cpp/src/arrow/array/validate.cc:960](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L960):24
    #5 0x5be0a3a714b2 in arrow::Status arrow::internal::ArraySpanInlineVisitor<arrow::Decimal32Type, void>::VisitStatus<arrow::Status arrow::internal::(anonymous namespace)::ValidateArrayImpl::ValidateDecimals<arrow::Decimal32Type>(arrow::Decimal32Type const&)::'lambda'(std::__1::basic_string_view<char, std::__1::char_traits<char>>), arrow::Status arrow::internal::(anonymous namespace)::ValidateArrayImpl::ValidateDecimals<arrow::Decimal32Type>(arrow::Decimal32Type const&)::'lambda'()>(arrow::ArraySpan const&, arrow::Decimal32Type&&, arrow::Status arrow::internal::(anonymous namespace)::ValidateArrayImpl::ValidateDecimals<arrow::Decimal32Type>(arrow::Decimal32Type const&)::'lambda'()&&)::'lambda'(long)::operator()(long) const [arrow/cpp/src/arrow/visit_data_inline.h:200](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/visit_data_inline.h#L200):18
    #6 0x5be0a3a5c805 in VisitBitBlocks<(lambda at /src/arrow/cpp/src/arrow/visit_data_inline.h:197:9), (lambda at /src/arrow/cpp/src/arrow/visit_data_inline.h:202:9)> [arrow/cpp/src/arrow/util/bit_block_counter.h:436](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/bit_block_counter.h#L436):9
    #7 0x5be0a3a5c805 in VisitStatus<(lambda at /src/arrow/cpp/src/arrow/array/validate.cc:957:11), (lambda at /src/arrow/cpp/src/arrow/array/validate.cc:966:11)> [arrow/cpp/src/arrow/visit_data_inline.h:195](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/visit_data_inline.h#L195):12
    #8 0x5be0a3a5c805 in VisitArraySpanInline<arrow::Decimal32Type, (lambda at /src/arrow/cpp/src/arrow/array/validate.cc:957:11), (lambda at /src/arrow/cpp/src/arrow/array/validate.cc:966:11)> [arrow/cpp/src/arrow/visit_data_inline.h:232](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/visit_data_inline.h#L232):10
    #9 0x5be0a3a5c805 in ValidateDecimals<arrow::Decimal32Type> [arrow/cpp/src/arrow/array/validate.cc:955](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L955):14
    #10 0x5be0a3a5c805 in arrow::internal::(anonymous namespace)::ValidateArrayImpl::Visit(arrow::Decimal32Type const&) [arrow/cpp/src/arrow/array/validate.cc:149](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L149):12
    #11 0x5be0a3a52cae in VisitTypeInline<arrow::internal::(anonymous namespace)::ValidateArrayImpl> [arrow/cpp/src/arrow/visit_type_inline.h:54](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/visit_type_inline.h#L54):5
    #12 0x5be0a3a52cae in arrow::internal::(anonymous namespace)::ValidateArrayImpl::ValidateWithType(arrow::DataType const&) [arrow/cpp/src/arrow/array/validate.cc:135](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L135):12
    #13 0x5be0a3a4c7da in arrow::internal::(anonymous namespace)::ValidateArrayImpl::Validate() [arrow/cpp/src/arrow/array/validate.cc:123](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L123):12
    #14 0x5be0a3a4d506 in ValidateArrayFull [arrow/cpp/src/arrow/array/validate.cc:990](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L990):60
    #15 0x5be0a3a4d506 in arrow::internal::ValidateArrayFull(arrow::Array const&) [arrow/cpp/src/arrow/array/validate.cc:994](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/array/validate.cc#L994):55
    #16 0x5be0a34eb8fd in arrow::(anonymous namespace)::ValidateBatch(arrow::RecordBatch const&, bool) [arrow/cpp/src/arrow/record_batch.cc:438](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/record_batch.cc#L438):39
    #17 0x5be0a34ec11c in arrow::RecordBatch::ValidateFull() const [arrow/cpp/src/arrow/record_batch.cc:694](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/record_batch.cc#L694):10
    #18 0x5be0a339602c in arrow::ipc::internal::(anonymous namespace)::ValidateFuzzBatch(arrow::RecordBatch const&) [arrow/cpp/src/arrow/ipc/reader.cc:2594](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/ipc/reader.cc#L2594):19
    #19 0x5be0a33967ce in arrow::ipc::internal::FuzzIpcFile(unsigned char const*, long) [arrow/cpp/src/arrow/ipc/reader.cc:2635](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/ipc/reader.cc#L2635):11
    #20 0x5be0a33708b3 in LLVMFuzzerTestOneInput [arrow/cpp/src/arrow/ipc/file_fuzz.cc:25](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/ipc/file_fuzz.cc#L25):17
    #21 0x5be0a32d2e80 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #22 0x5be0a32be0f5 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #23 0x5be0a32c3b8f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #24 0x5be0a32eee32 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #25 0x7edec1224082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
    #26 0x5be0a32b62dd in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior [arrow/cpp/src/arrow/util/basic_decimal.h:280](https://github.com/apache/arrow/blob/f41f59066b79fbf59719e68ef0f908afd6c5218c/cpp/src/arrow/util/basic_decimal.h#L280):14

[pitrou]

In this case I think the safety checks deserve to be in FitsInPrecision rather than in Negate.

[pitrou]

(also should check how Decimal128 and Decimal256 behave in similar cases?)

[zeroshade]

Are you thinking just a simple if (value_ == INT32_MIN) { return false; } check in FitsInPrecision?

[zeroshade]

both Decimal128 and Decimal256 use the SafeSigned* variants and manually perform two's complement negation via ~bits + 1 etc. So they wouldn't run into this issue

[pitrou]

Are you thinking just a simple if (value_ == INT32_MIN) { return false; } check in FitsInPrecision?

I was thinking that indeed. However, we may also want to make Negate safe if it's safe in Decimal128/256. But this means we might have to move the Negate implementation to basic_decimal.cc, depending on CI results.

[zeroshade]

@pitrou Current version work for you?

[pitrou]

@zeroshade Can you open a PR to add the regression file in https://github.com/apache/arrow-testing/tree/master/data/arrow-ipc-file ? It will then get picked up by the ASAN/UBSAN CI job.

[pitrou]

LGTM, but should update the arrow-testing submodule once the regression file is checked in there.

[zeroshade]

Added apache/arrow-testing#104 though I still wasn't able to reproduce the issue with the downloaded clusterfuzz file. I only worked it out by reading through the reported stack trace.

[pitrou]

Added apache/arrow-testing#104 though I still wasn't able to reproduce the issue with the downloaded clusterfuzz file.

Unfortunately that can depend on the compiler version and other details. But it's still good to have it somewhere.

[pitrou]

Thanks a lot @zeroshade !

@amoeba This is a good candidate for 19.0.0 if not too late.

[amoeba]

It's too late for RC0 but I'll mark the issue in case we do an RC1.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b1bb480.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.