[R][C++] Undefined behaviour (clang sanitizer) in decompression

[paleolimbot]

Describe the bug, including details regarding any error messages, version, and platform.

From the latest test-fedora-r-clang-sanitizer nightly:

Start test: read/write compressed file successfully
  'test-csv.R:614:3' [success]
  'test-csv.R:616:3' [success]
  'test-csv.R:623:3' [success]
/tmp/RtmpMpfJ3z/file105578298f8/lz4_ep-prefix/src/lz4_ep/lib/lz4frame.c:1563:41: runtime error: applying zero offset to null pointer
    #0 0x7fe873594f04 in LZ4F_decompress /tmp/RtmpMpfJ3z/file105578298f8/lz4_ep-prefix/src/lz4_ep/lib/lz4frame.c:1563:41
    #1 0x7fe87006bad4 in arrow::util::internal::(anonymous namespace)::LZ4Decompressor::Decompress(long, unsigned char const*, long, unsigned char*) /arrow/cpp/src/arrow/util/compression_lz4.cc:113:9
    #2 0x7fe86fd1f024 in arrow::io::CompressedInputStream::Impl::DecompressData() /arrow/cpp/src/arrow/io/compressed.cc:294:7
    #3 0x7fe86fd1e0d4 in arrow::io::CompressedInputStream::Impl::RefillDecompressed(bool*) /arrow/cpp/src/arrow/io/compressed.cc:338:7
    #4 0x7fe86fd199bd in arrow::io::CompressedInputStream::Impl::Read(long, void*) /arrow/cpp/src/arrow/io/compressed.cc:372:7
    #5 0x7fe86fd19ec1 in arrow::io::CompressedInputStream::Impl::Read(long) /arrow/cpp/src/arrow/io/compressed.cc:381:5
    #6 0x7fe86fd14348 in arrow::io::CompressedInputStream::DoRead(long) /arrow/cpp/src/arrow/io/compressed.cc:435:17
    #7 0x7fe86fd1a8d1 in arrow::io::internal::InputStreamConcurrencyWrapper<arrow::io::CompressedInputStream>::Read(long) /arrow/cpp/src/arrow/io/concurrency.h:116:23
    #8 0x7fe86fd4f1c5 in arrow::io::(anonymous namespace)::InputStreamBlockIterator::Next() /arrow/cpp/src/arrow/io/interfaces.cc:89:5
    #9 0x7fe86fd4f1c5 in arrow::Result<std::__1::shared_ptr<arrow::Buffer> > arrow::Iterator<std::__1::shared_ptr<arrow::Buffer> >::Next<arrow::io::(anonymous namespace)::InputStreamBlockIterator>(void*) /arrow/cpp/src/arrow/util/iterator.h:200:40
    #10 0x7fe87013d8af in arrow::Iterator<std::__1::shared_ptr<arrow::Buffer> >::Next() /arrow/cpp/src/arrow/util/iterator.h:110:29
    #11 0x7fe87013c8d9 in arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::WorkerTask(std::__1::shared_ptr<arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::State>) /arrow/cpp/src/arrow/util/async_generator.h:1759:29
    #12 0x7fe87013c5ac in arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::State::DoRestartTask(std::__1::shared_ptr<arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::State>, arrow::util::Mutex::Guard)::'lambda'()::operator()() const /arrow/cpp/src/arrow/util/async_generator.h:1666:23
    #13 0x7fe87013c5ac in arrow::internal::FnOnce<void ()>::FnImpl<arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::State::DoRestartTask(std::__1::shared_ptr<arrow::BackgroundGenerator<std::__1::shared_ptr<arrow::Buffer> >::State>, arrow::util::Mutex::Guard)::'lambda'()>::invoke() /arrow/cpp/src/arrow/util/functional.h:152:42
    #14 0x7fe86ffa304b in arrow::internal::FnOnce<void ()>::operator()() && /arrow/cpp/src/arrow/util/functional.h:140:17

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /tmp/RtmpMpfJ3z/file105578298f8/lz4_ep-prefix/src/lz4_ep/lib/lz4frame.c:1563:41 in 
  'test-csv.R:624:3' [success]
End test: read/write compressed file successfully

Component(s)

R

[paleolimbot]

The offending code is basically:

library(arrow, warn.conflicts = FALSE)

tbl <- data.frame(
  dbl = c(1:8, NA, 10) + .1,
  lgl = sample(c(TRUE, FALSE, NA), 10, replace = TRUE),
  false = logical(10),
  chr = letters[c(1:5, NA, 7:10)]
)

tflz4 <- tempfile(fileext = ".csv.lz4")
write_csv_arrow(tbl, tflz4)
read_csv_arrow(tflz4)
#>     dbl   lgl false  chr
#> 1   1.1  TRUE FALSE    a
#> 2   2.1 FALSE FALSE    b
#> 3   3.1 FALSE FALSE    c
#> 4   4.1  TRUE FALSE    d
#> 5   5.1  TRUE FALSE    e
#> 6   6.1 FALSE FALSE <NA>
#> 7   7.1    NA FALSE    g
#> 8   8.1 FALSE FALSE    h
#> 9    NA    NA FALSE    i
#> 10 10.1 FALSE FALSE    j

I don't have a sanitizer build set up locally and may need some help!

[bkietz]

This looks like an empty buffer happens to be read for decompression, so a null pointer is used since there's no data to read. Even though the length of the buffer is zero, it's still being recognized as undefined behavior. That's odd to me since null pointer arithmetic with a zero offset is not UB in c++17 and the lz4 external project should inherit that. I'm not familiar enough with C to know if this differs there.

In any case, it seems the fix should be to explicitly check for null in Lz4Decompressor and skip passing that through to LZ4F_decompress. Possibly we should also look for the Readable implementation which is producing a null/empty buffer in the first place; patching that to return a pointer to kZeroSizeArea instead will prevent this kind of error next time. Actually, @pitrou what would you think of changing Buffer's constructor such that data_ would never be nullptr?

[pitrou]

I think that first we should find out why the decompressor is being called with a null pointer. Decompressed data is typically never empty, so this looks like a bug in CompressedInputStream.

[pitrou]

Also, can you find out at which point this started failing @paleolimbot ?

[paleolimbot]

Thank you for taking a look!

It looks like the last successful build was Oct 24 at 9:25 p.m...the only commit that jumps out at me is an R build system PR and some compression benchmarks, although the compression benchmarks seems to just be benchmarks.

@assignUser the R install output is slightly different between the last passing run and the first failing run although it seems to be that the end result is the same (build libarrow from source).

https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=56301&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=3277

https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=56235&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=3253

[assignUser]

Hm I thought maybe the used lz4 version is different for some reason but it looks like both built it from source using version 1.9.4. The build system change has activated more features (gcs/s3, jemalloc, bz2, zlib, zstd, ...) by default which is likely the reason this is erroring now, can't really help with why though :/

[paleolimbot]

Thanks, that helps: it started failing then because the build system change activated either gzip or lz4 on this build that hadn't been previously activated. Previously, this test just didn't run on the sanitizer build.

[pitrou]

I'll probably won't be able to reproduce locally (unless you can show me a simple way of doing with Docker / archery), but I can start from the traceback and try to diagnose from that.

[pitrou]

@paleolimbot Does this still happen?

Edit: it does.