Skip to content

Commit 62e3794

Browse files
authored
fix: LZ4 block compressor/decompressor safety issues (alibaba#344)
1 parent 07118c5 commit 62e3794

4 files changed

Lines changed: 45 additions & 2 deletions

File tree

src/paimon/common/compression/block_compression_factory_test.cpp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,39 @@ TEST_P(CompressionFactoryTest, TESTCompressThenDecompress) {
7979
ASSERT_EQ(0, decompressor->ReadIntLE(read_write_le.data()));
8080
}
8181

82+
TEST_P(CompressionFactoryTest, TestDecompressTruncatedHeader) {
83+
BlockCompressionType type = GetParam();
84+
ASSERT_OK_AND_ASSIGN(auto factory, BlockCompressionFactory::Create(type));
85+
auto decompressor = factory->GetDecompressor();
86+
87+
// Source shorter than 8-byte header should return Invalid, not read out of bounds
88+
char short_buf[] = {0x01, 0x02, 0x03};
89+
char dst[64] = {};
90+
ASSERT_NOK(decompressor->Decompress(short_buf, 3, dst, sizeof(dst)));
91+
}
92+
93+
TEST_P(CompressionFactoryTest, TestCompressInsufficientOutputBuffer) {
94+
BlockCompressionType type = GetParam();
95+
ASSERT_OK_AND_ASSIGN(auto factory, BlockCompressionFactory::Create(type));
96+
auto compressor = factory->GetCompressor();
97+
98+
// Incompressible data with a tiny output buffer should fail, not produce a corrupt block
99+
std::string data(1024, '\0');
100+
for (int32_t i = 0; i < static_cast<int32_t>(data.size()); i++) {
101+
data[i] = static_cast<char>(i % 251);
102+
}
103+
// Output buffer too small: only HEADER_LENGTH bytes, no room for compressed payload
104+
std::string tiny_dst(8, '\0');
105+
ASSERT_NOK(compressor->Compress(data.data(), data.size(), tiny_dst.data(), tiny_dst.size()));
106+
107+
// Output buffer smaller than HEADER_LENGTH — must not trigger UB from negative capacity
108+
char micro_dst[4] = {};
109+
ASSERT_NOK(compressor->Compress(data.data(), data.size(), micro_dst, sizeof(micro_dst)));
110+
111+
// Zero-length output buffer
112+
ASSERT_NOK(compressor->Compress(data.data(), data.size(), nullptr, 0));
113+
}
114+
82115
INSTANTIATE_TEST_SUITE_P(BlockCompressionTypeGroup, CompressionFactoryTest,
83116
::testing::Values(BlockCompressionType::LZ4, BlockCompressionType::ZSTD));
84117

src/paimon/common/compression/lz4/lz4_block_compressor.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,15 @@ class Lz4BlockCompressor : public BlockCompressor {
3232

3333
Result<int32_t> Compress(const char* src, int32_t src_length, char* dst,
3434
int32_t dst_length) override {
35+
if (dst_length < BlockCompressor::HEADER_LENGTH) {
36+
return Status::Invalid(fmt::format(
37+
"Output buffer too small for LZ4 block header, expected at least {} bytes, got {}",
38+
BlockCompressor::HEADER_LENGTH, dst_length));
39+
}
3540
int32_t compressed_size =
3641
LZ4_compress_default(src, dst + BlockCompressor::HEADER_LENGTH, src_length,
3742
dst_length - BlockCompressor::HEADER_LENGTH);
38-
if (compressed_size < 0) {
43+
if (compressed_size <= 0) {
3944
return Status::Invalid(fmt::format("Compression failed with code {}", compressed_size));
4045
}
4146
WriteIntLE(compressed_size, dst);

src/paimon/common/compression/lz4/lz4_block_decompressor.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ class Lz4BlockDecompressor : public BlockDecompressor {
2828
public:
2929
Result<int32_t> Decompress(const char* src, int32_t src_length, char* dst,
3030
int32_t dst_length) override {
31+
if (src_length < HEADER_LENGTH) {
32+
return Status::Invalid(fmt::format(
33+
"Source data too short for LZ4 block header, expected at least {} bytes, got {}",
34+
HEADER_LENGTH, src_length));
35+
}
3136
auto compressed_len = ReadIntLE(src);
3237
auto original_len = ReadIntLE(src + 4);
3338
PAIMON_RETURN_NOT_OK(ValidateLength(compressed_len, original_len));

src/paimon/format/parquet/predicate_pushdown_test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ TEST_F(PredicatePushdownTest, TestStringData) {
343343
CheckResult(read_schema, predicate, /*expected_array=*/expected_array);
344344
}
345345
{
346-
// f0 like 'me', no data
346+
// f0 like 'me', has data
347347
ASSERT_OK_AND_ASSIGN(const auto predicate,
348348
PredicateBuilder::Like(
349349
/*field_index=*/0, /*field_name=*/"f0", FieldType::STRING,

0 commit comments

Comments
 (0)