Fixing crashes with malformed compressed data

The Gorilla and Dictionary compressors crashed on malformed data
with the reverse decompression iterator and the bulk decompressor.

This issue was reported by Mehmet Ince @mdisec https://mehmetince.net/,
and the test case binaries are his contribution that I integrated into
the fuzzer test suite.

This change adds validation for these cases and adds reverse iterator
coverage to the fuzzer tests, so the randomised test will catch future
issues. It already caught issues in other compressors like deltadelta,
which I also fixed in this review.

Author: dbeck

.unreleased/pr_10360

@@ -0,0 +1,2 @@
+Fixes: #10360 Decompressor crashes with malformed compressed data
+Thanks: @mdisec for reporting the issues with the validation of the compressed data during decompression

src/adts/bit_array.h

@@ -37,6 +37,7 @@ pg_attribute_always_inline static uint64 bit_array_iter_next(BitArrayIterator *i
 static void bit_array_iterator_init_rev(BitArrayIterator *iter, const BitArray *array);
 /* return last num_bits in forward-order (not reverse-order); must have been written as num_bits */
 static uint64 bit_array_iter_next_rev(BitArrayIterator *iter, uint8 num_bits);
+static inline uint64 bit_array_iter_position(const BitArrayIterator *iter);
 
 /* I/O */
 static inline void bit_array_send(StringInfo buffer, const BitArray *data);

src/adts/bit_array_impl.h

@@ -85,8 +85,10 @@ bit_array_num_buckets(const BitArray *array)
 static inline uint64
 bit_array_num_bits(const BitArray *array)
 {
-	return (BITS_PER_BUCKET * (array->buckets.num_elements - UINT64CONST(1))) +
-		   array->bits_used_in_last_bucket;
+	return array->buckets.num_elements == 0 ?
+			   0 :
+			   (BITS_PER_BUCKET * (array->buckets.num_elements - UINT64CONST(1))) +
+				   array->bits_used_in_last_bucket;
 }
 
 static inline uint64 *
@@ -250,6 +252,15 @@ bit_array_iterator_init(BitArrayIterator *iter, const BitArray *array)
 	};
 }
 
+static inline uint64
+bit_array_iter_position(const BitArrayIterator *iter)
+{
+	Assert(iter->current_bucket >= 0);
+	return iter->current_bucket < 0 ?
+			   0 :
+			   (BITS_PER_BUCKET * iter->current_bucket) + iter->bits_used_in_current_bucket;
+}
+
 pg_attribute_always_inline static uint64
 bit_array_iter_next(BitArrayIterator *iter, uint8 num_bits)
 {
@@ -302,7 +313,7 @@ bit_array_iterator_init_rev(BitArrayIterator *iter, const BitArray *array)
 {
 	*iter = (BitArrayIterator){
 		.array = array,
-		.current_bucket = array->buckets.num_elements - 1,
+		.current_bucket = array->buckets.num_elements == 0 ? 0 : array->buckets.num_elements - 1,
 		.bits_used_in_current_bucket = array->bits_used_in_last_bucket,
 	};
 }

tsl/src/compression/algorithms/deltadelta.c

@@ -481,6 +481,44 @@ delta_delta_compressor_append_value(DeltaDeltaCompressor *compressor, int64 next
 /**********************************************************************************/
 /**********************************************************************************/
 
+static inline uint32
+decompression_iterator_items_seen(const DeltaDeltaDecompressionIterator *iter)
+{
+	if (iter->has_nulls)
+	{
+		return iter->nulls.num_elements_returned;
+	}
+	else
+	{
+		return iter->delta_deltas.num_elements_returned;
+	}
+}
+
+static inline uint32
+decompression_iterator_item_count(const DeltaDeltaDecompressionIterator *iter)
+{
+	if (iter->has_nulls)
+	{
+		return iter->nulls.num_elements;
+	}
+	else
+	{
+		return iter->delta_deltas.num_elements;
+	}
+}
+
+static inline uint32
+decompression_iterator_values_seen(const DeltaDeltaDecompressionIterator *iter)
+{
+	return iter->delta_deltas.num_elements_returned;
+}
+
+static inline uint32
+decompression_iterator_value_count(const DeltaDeltaDecompressionIterator *iter)
+{
+	return iter->delta_deltas.num_elements;
+}
+
 static void
 int64_decompression_iterator_init_forward(DeltaDeltaDecompressionIterator *iter, void *compressed,
 										  Oid element_type)
@@ -515,6 +553,7 @@ int64_decompression_iterator_init_forward(DeltaDeltaDecompressionIterator *iter,
 	{
 		Simple8bRleSerialized *nulls = bytes_deserialize_simple8b_and_advance(&si);
 		simple8brle_decompression_iterator_init_forward(&iter->nulls, nulls);
+		CheckCompressedData(deltas->num_elements <= nulls->num_elements);
 	}
 }
 
@@ -529,7 +568,7 @@ int64_decompression_iterator_init_reverse(DeltaDeltaDecompressionIterator *iter,
 	DeltaDeltaCompressed *header = consumeCompressedData(&si, sizeof(DeltaDeltaCompressed));
 	Simple8bRleSerialized *deltas = bytes_deserialize_simple8b_and_advance(&si);
 
-	Assert(header->has_nulls == 0 || header->has_nulls == 1);
+	CheckCompressedData(header->has_nulls == 0 || header->has_nulls == 1);
 
 	*iter = (DeltaDeltaDecompressionIterator){
 		.base = {
@@ -549,6 +588,7 @@ int64_decompression_iterator_init_reverse(DeltaDeltaDecompressionIterator *iter,
 	{
 		Simple8bRleSerialized *nulls = bytes_deserialize_simple8b_and_advance(&si);
 		simple8brle_decompression_iterator_init_reverse(&iter->nulls, nulls);
+		CheckCompressedData(deltas->num_elements <= nulls->num_elements);
 	}
 }
 
@@ -615,6 +655,12 @@ delta_delta_decompression_iterator_try_next_forward_internal(DeltaDeltaDecompres
 			simple8brle_decompression_iterator_try_next_forward(&iter->nulls);
 		if (result.is_done)
 		{
+			/* make sure we exhausted all items before  */
+			CheckCompressedData(decompression_iterator_items_seen(iter) ==
+								decompression_iterator_item_count(iter));
+			/* and also that we returned all values */
+			CheckCompressedData(decompression_iterator_values_seen(iter) ==
+								decompression_iterator_value_count(iter));
 			return (DecompressResultInternal){
 				.is_done = true,
 			};
@@ -633,6 +679,12 @@ delta_delta_decompression_iterator_try_next_forward_internal(DeltaDeltaDecompres
 
 	if (result.is_done)
 	{
+		/* make sure we exhausted all items before  */
+		CheckCompressedData(decompression_iterator_items_seen(iter) ==
+							decompression_iterator_item_count(iter));
+		/* and also that we returned all values */
+		CheckCompressedData(decompression_iterator_values_seen(iter) ==
+							decompression_iterator_value_count(iter));
 		return (DecompressResultInternal){
 			.is_done = true,
 		};
@@ -712,14 +764,20 @@ delta_delta_decompression_iterator_try_next_reverse_internal(DeltaDeltaDecompres
 			simple8brle_decompression_iterator_try_next_reverse(&iter->nulls);
 		if (result.is_done)
 		{
+			/* make sure we exhausted all items before  */
+			CheckCompressedData(decompression_iterator_items_seen(iter) ==
+								decompression_iterator_item_count(iter));
+			/* and also that we returned all values */
+			CheckCompressedData(decompression_iterator_values_seen(iter) ==
+								decompression_iterator_value_count(iter));
 			return (DecompressResultInternal){
 				.is_done = true,
 			};
 		}
 
 		if (result.val != 0)
 		{
-			Assert(result.val == 1);
+			CheckCompressedData(result.val == 1);
 			return (DecompressResultInternal){
 				.is_null = true,
 			};
@@ -730,6 +788,12 @@ delta_delta_decompression_iterator_try_next_reverse_internal(DeltaDeltaDecompres
 
 	if (result.is_done)
 	{
+		/* make sure we exhausted all items before  */
+		CheckCompressedData(decompression_iterator_items_seen(iter) ==
+							decompression_iterator_item_count(iter));
+		/* and also that we returned all values */
+		CheckCompressedData(decompression_iterator_values_seen(iter) ==
+							decompression_iterator_value_count(iter));
 		return (DecompressResultInternal){
 			.is_done = true,
 		};

tsl/src/compression/algorithms/dictionary.c

@@ -719,7 +719,8 @@ tsl_text_dictionary_decompress_all(Datum compressed, Oid element_type, MemoryCon
 	bool have_incorrect_index = false;
 	for (uint32 i = 0; i < n_notnull; i++)
 	{
-		have_incorrect_index = have_incorrect_index || indices[i] >= (int16) header->num_distinct;
+		have_incorrect_index =
+			have_incorrect_index || indices[i] >= (int16) header->num_distinct || indices[i] < 0;
 	}
 	CheckCompressedData(!have_incorrect_index);
 
@@ -897,7 +898,7 @@ dictionary_decompression_iterator_try_next_reverse(DecompressionIterator *iter_b
 		};
 	}
 
-	Assert(result.val < iter->compressed->num_distinct);
+	CheckCompressedData(result.val < iter->compressed->num_distinct);
 	return (DecompressResult){
 		.val = iter->values[result.val],
 		.is_null = false,

tsl/src/compression/algorithms/gorilla.c

@@ -519,6 +519,44 @@ gorilla_compressor_finish(GorillaCompressor *compressor)
  ***  DecompressionIterator  ***
  *******************************/
 
+static inline uint32
+decompression_iterator_items_seen(const GorillaDecompressionIterator *iter)
+{
+	if (iter->has_nulls)
+	{
+		return iter->nulls.num_elements_returned;
+	}
+	else
+	{
+		return iter->tag0s.num_elements_returned;
+	}
+}
+
+static inline uint32
+decompression_iterator_item_count(const GorillaDecompressionIterator *iter)
+{
+	if (iter->has_nulls)
+	{
+		return iter->nulls.num_elements;
+	}
+	else
+	{
+		return iter->tag0s.num_elements;
+	}
+}
+
+static inline uint32
+decompression_iterator_values_seen(const GorillaDecompressionIterator *iter)
+{
+	return iter->tag0s.num_elements_returned;
+}
+
+static inline uint32
+decompression_iterator_value_count(const GorillaDecompressionIterator *iter)
+{
+	return iter->tag0s.num_elements;
+}
+
 inline static void
 bytes_attach_bit_array_and_advance(BitArray *dst, StringInfo si, uint32 num_buckets,
 								   uint8 bits_in_last_bucket)
@@ -560,11 +598,29 @@ compressed_gorilla_data_init_from_stringinfo(CompressedGorillaData *expanded, St
 	if (has_nulls)
 	{
 		expanded->nulls = bytes_deserialize_simple8b_and_advance(si);
+		CheckCompressedData(expanded->nulls->num_elements >=
+							expanded->num_bits_used_per_xor->num_elements);
+		CheckCompressedData(expanded->nulls->num_elements >= expanded->tag0s->num_elements);
+		CheckCompressedData(expanded->nulls->num_elements >= expanded->tag1s->num_elements);
 	}
 	else
 	{
 		expanded->nulls = NULL;
 	}
+
+	/* XOR bit count must be reasonable */
+	uint64 xor_bit_count = bit_array_num_bits(&expanded->xors);
+	CheckCompressedData(xor_bit_count <= expanded->tag0s->num_elements * 64);
+
+	/* leading zeros have a fixed size and we need at least one item */
+	uint64 num_leading_zeros_bits = bit_array_num_bits(&expanded->leading_zeros);
+	CheckCompressedData(num_leading_zeros_bits >= BITS_PER_LEADING_ZEROS &&
+						num_leading_zeros_bits % BITS_PER_LEADING_ZEROS == 0 &&
+						num_leading_zeros_bits <=
+							expanded->tag0s->num_elements * BITS_PER_LEADING_ZEROS);
+
+	/* tag bits must be reasonable too */
+	CheckCompressedData(expanded->tag0s->num_elements >= expanded->tag1s->num_elements);
 }
 
 static void
@@ -672,6 +728,12 @@ gorilla_decompression_iterator_try_next_forward_internal(GorillaDecompressionIte
 		/* Could slightly improve performance here by not returning a tail of non-null bits */
 		if (null.is_done)
 		{
+			/* make sure we exhausted all items before  */
+			CheckCompressedData(decompression_iterator_items_seen(iter) ==
+								decompression_iterator_item_count(iter));
+			/* and also that we returned all values */
+			CheckCompressedData(decompression_iterator_values_seen(iter) ==
+								decompression_iterator_value_count(iter));
 			return (DecompressResultInternal){
 				.is_done = true,
 			};
@@ -689,6 +751,12 @@ gorilla_decompression_iterator_try_next_forward_internal(GorillaDecompressionIte
 	/* if we don't have a null bitset, this will determine when we're done */
 	if (tag0.is_done)
 	{
+		/* make sure we exhausted all items before  */
+		CheckCompressedData(decompression_iterator_items_seen(iter) ==
+							decompression_iterator_item_count(iter));
+		/* and also that we returned all values */
+		CheckCompressedData(decompression_iterator_values_seen(iter) ==
+							decompression_iterator_value_count(iter));
 		CheckCompressedData(!iter->has_nulls);
 		return (DecompressResultInternal){
 			.is_done = true,
@@ -799,7 +867,7 @@ gorilla_decompression_iterator_from_datum_reverse(Datum gorilla_compressed, Oid
 	iter->prev_leading_zeroes =
 		bit_array_iter_next_rev(&iter->leading_zeros, BITS_PER_LEADING_ZEROS);
 	num_xor_bits = simple8brle_decompression_iterator_try_next_reverse(&iter->num_bits_used);
-	Assert(!num_xor_bits.is_done);
+	CheckCompressedData(!num_xor_bits.is_done);
 	iter->prev_xor_bits_used = num_xor_bits.val;
 	iter->prev_val = iter->gorilla_data.header->last_value;
 	return &iter->base;
@@ -820,6 +888,12 @@ gorilla_decompression_iterator_try_next_reverse_internal(GorillaDecompressionIte
 
 		if (null.is_done)
 		{
+			/* make sure we exhausted all items before  */
+			CheckCompressedData(decompression_iterator_items_seen(iter) ==
+								decompression_iterator_item_count(iter));
+			/* and also that we returned all values */
+			CheckCompressedData(decompression_iterator_values_seen(iter) ==
+								decompression_iterator_value_count(iter));
 			return (DecompressResultInternal){
 				.is_done = true,
 			};
@@ -839,6 +913,12 @@ gorilla_decompression_iterator_try_next_reverse_internal(GorillaDecompressionIte
 	/* if we don't have a null bitset, this will determine when we're done */
 	if (tag0.is_done)
 	{
+		/* make sure we exhausted all items before  */
+		CheckCompressedData(decompression_iterator_items_seen(iter) ==
+							decompression_iterator_item_count(iter));
+		/* and also that we returned all values */
+		CheckCompressedData(decompression_iterator_values_seen(iter) ==
+							decompression_iterator_value_count(iter));
 		return (DecompressResultInternal){
 			.is_done = true,
 		};
@@ -851,6 +931,13 @@ gorilla_decompression_iterator_try_next_reverse_internal(GorillaDecompressionIte
 		};
 	}
 
+	/* make sure we are in a valid state */
+	tag1 = simple8brle_decompression_iterator_try_next_reverse(&iter->tag1s);
+	CheckCompressedData(!tag1.is_done);
+
+	/* check that we have this much to read */
+	CheckCompressedData(iter->prev_leading_zeroes + iter->prev_xor_bits_used > 0);
+	CheckCompressedData(bit_array_iter_position(&iter->xors) >= iter->prev_xor_bits_used);
 	xor = bit_array_iter_next_rev(&iter->xors, iter->prev_xor_bits_used);
 
 	if (iter->prev_leading_zeroes + iter->prev_xor_bits_used < 64)
@@ -859,8 +946,6 @@ gorilla_decompression_iterator_try_next_reverse_internal(GorillaDecompressionIte
 	}
 	iter->prev_val ^= xor;
 
-	tag1 = simple8brle_decompression_iterator_try_next_reverse(&iter->tag1s);
-
 	if (tag1.val != 0)
 	{
 		/* get new xor sizes */
@@ -875,9 +960,15 @@ gorilla_decompression_iterator_try_next_reverse_internal(GorillaDecompressionIte
 		}
 		else
 		{
+			/* make sure we have enough bits to read for the leading zeros */
+			CheckCompressedData(bit_array_iter_position(&iter->leading_zeros) >=
+								BITS_PER_LEADING_ZEROS);
 			iter->prev_xor_bits_used = num_xor_bits.val;
 			iter->prev_leading_zeroes =
 				bit_array_iter_next_rev(&iter->leading_zeros, BITS_PER_LEADING_ZEROS);
+
+			/* more than 64 bits of data doesn't make sense */
+			CheckCompressedData(iter->prev_leading_zeroes + iter->prev_xor_bits_used <= 64);
 		}
 	}