[SCM] Samba Shared Repository - branch master updated
Jeremy Allison
jra at samba.org
Mon Dec 19 23:30:01 UTC 2022
The branch, master has been updated
via 44a44005a6b compression/huffman: debug function bails upon disaster (CID 1517261)
via 628f14c1497 compression/huffman: double check distance in matches (CID 1517278)
via 6b4d94c9877 compression: fix sign extension of long matches (CID 1517275)
via baba440ffaa compression tests: avoid div by zero in failure (CID 1517297)
via b99e0e9301d compression/tests: calm the static analysts (CID: numerous)
via d6a67908e13 compression/huffman: check again for invalid codes (CID 1517302)
via 27af27f9018 compression/huffman: tighten bit_len checks (fix SUSE -O3 build)
via e7489be7be4 fuzz: fix lzxpress plain round-trip fuzzer
via 6f77b376d47 compression/huffman: avoid semi-defined behaviour in decompress
from 80c0b416892 rpc_server:srvsvc - retrieve share ACL via root context
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit 44a44005a6b3222c599d4757d60af924b9cea459
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Dec 7 09:27:00 2022 +1300
compression/huffman: debug function bails upon disaster (CID 1517261)
We shouldn't get a node with a zero code, and there's probably nothing
to do but stop.
CID 1517261 (#1-2 of 2): Bad bit shift operation
(BAD_SHIFT)11. negative_shift: In expression j >> offset - k,
shifting by a negative amount has undefined behavior. The shift
amount, offset - k, is -3.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
Autobuild-User(master): Jeremy Allison <jra at samba.org>
Autobuild-Date(master): Mon Dec 19 23:29:04 UTC 2022 on sn-devel-184
commit 628f14c149772dc4277c004018b8f02420fa3997
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Dec 7 09:17:17 2022 +1300
compression/huffman: double check distance in matches (CID 1517278)
Because we just wrote the intermediate representation to have no zero
distances, we can be sure it doesn't, but Coverity doesn't know. If
distance is zero, `bitlen_nonzero_16(distance)` would be bad.
CID 1517278 (#1 of 1): Bad bit shift operation
(BAD_SHIFT)41. large_shift: In expression 1 << code_dist, left
shifting by more than 31 bits has undefined behavior. The shift
amount, code_dist, is 65535.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit 6b4d94c9877ec59081b9da946c00fa2647cad928
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Dec 7 09:08:11 2022 +1300
compression: fix sign extension of long matches (CID 1517275)
Very long matches would be written instead as very very long matches.
We can't in fact hit this because we have a MAX_MATCH_LENGTH defined
as 64M, but if we could, it might make certain 2GB+ strings impossible
to compress.
CID 1517275 (#1 of 1): Unintended sign extension
(SIGN_EXTENSION)sign_extension: Suspicious implicit sign extension:
intermediate[i + 2UL] with type uint16_t (16 bits, unsigned) is
promoted in intermediate[i + 2UL] << 16 to type int (32 bits, signed),
then sign-extended to type unsigned long (64 bits, unsigned). If
intermediate[i + 2UL] << 16 is greater than 0x7FFFFFFF, the upper bits
of the result will all be 1.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit baba440ffaaf849e14e31862649767227e8c6432
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Tue Dec 6 11:28:58 2022 +1300
compression tests: avoid div by zero in failure (CID 1517297)
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit b99e0e9301d14574ed24181ce300ea61558d4d02
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Tue Dec 6 11:26:47 2022 +1300
compression/tests: calm the static analysts (CID: numerous)
None of our test vectors are 18446744073709551615 bytes long, which
means we can know an `expected_length == returned_length` check will
catch the case where the compression function returns -1 for error. We
know that, but Coverity doesn't.
It's the same thing over and over again, in two different patterns:
>>> CID 1517301: Memory - corruptions (OVERRUN)
>>> Calling "memcmp" with "original.data" and "original.length" is
suspicious because of the very large index, 18446744073709551615. The index
may be due to a negative parameter being interpreted as unsigned.
393 if (original.length != decomp_written ||
394 memcmp(decompressed.data,
395 original.data,
396 original.length) != 0) {
397 debug_message("\033[1;31mgot %zd, expected %zu\033[0m\n",
398 decomp_written,
*** CID 1517299: Memory - corruptions (OVERRUN)
/lib/compression/tests/test_lzxpress_plain.c: 296 in
test_lzxpress_plain_decompress_more_compressed_files()
290 debug_start_timer();
291 written = lzxpress_decompress(p.compressed.data,
292 p.compressed.length,
293 dest,
294 p.decompressed.length);
295 debug_end_timer("decompress", p.decompressed.length);
>>> CID 1517299: Memory - corruptions (OVERRUN)
>>> Calling "memcmp" with "p.decompressed.data" and
"p.decompressed.length" is suspicious because of the very large index,
18446744073709551615. The index may be due to a negative parameter being
interpreted as unsigned.
296 if (written == p.decompressed.length &&
297 memcmp(dest, p.decompressed.data, p.decompressed.length)
== 0) {
298 debug_message("\033[1;32mdecompressed %s!
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit d6a67908e13dd46b3bd336adae97e26920bb7f90
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Tue Dec 6 11:24:22 2022 +1300
compression/huffman: check again for invalid codes (CID 1517302)
We know that code is non-zero, because it comes from the combination of
the intermediate representation and the symbol tables that were generated
at the same time. But Coverity doesn't know that, and it thinks we could
be doing undefined things in the subsequent shift.
CID 1517302: Integer handling issues (BAD_SHIFT)
In expression "1 << code_bit_len", shifting by a negative amount has
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit 27af27f9018b8bf32eac8ae79401354f6f18a4c6
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Dec 7 12:01:32 2022 +1300
compression/huffman: tighten bit_len checks (fix SUSE -O3 build)
The struct write_context bit_len attribute is always between 0 and 31,
but if the next patches are applied without this, SUSE GCC -O3 will
worry thusly:
../../lib/compression/lzxpress_huffman.c: In function
‘lzxpress_huffman_compress’:
../../lib/compression/lzxpress_huffman.c:953:5: error: assuming signed
overflow does not occur when simplifying conditional to constant
[-Werror=strict-overflow]
if (wc->bit_len > 16) {
^
cc1: all warnings being treated as errors
Inspection tell us that the invariant holds. Nevertheless, we can
safely use an unsigned type and insist that over- or under- flow is
bad.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit e7489be7be4d05a75a7d31275654260f84a64c79
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun Dec 4 11:47:56 2022 +1300
fuzz: fix lzxpress plain round-trip fuzzer
The 'compressed' string can be about 9/8 the size of the decompressed
string, but we didn't allow enough memory in the fuzz target for that.
Then when it failed, we didn't check.
Credit to OSSFuzz.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
commit 6f77b376d470dd318f0a9699b3528018ce8ea49a
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun Dec 4 11:33:29 2022 +1300
compression/huffman: avoid semi-defined behaviour in decompress
We had
output[output_pos - distance];
where output_pos and distance are size_t and distance can be greater
than output_pos (because it refers to a place in the previous block).
The underflow is defined, leading to a big number, and when
sizeof(size_t) == sizeof(*uint8_t) the subsequent overflow works as
expected. But if size_t is smaller than a pointer, bad things will
happen.
This was found by OSSFuzz with
'UBSAN_OPTIONS=print_stacktrace=1:silence_unsigned_overflow=1'.
Credit to OSSFuzz.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
-----------------------------------------------------------------------
Summary of changes:
lib/compression/lzxpress_huffman.c | 30 +++++++++++++++++++++--------
lib/compression/tests/test_lzx_huffman.c | 6 +++++-
lib/compression/tests/test_lzxpress_plain.c | 22 +++++++++++++++++----
lib/fuzzing/fuzz_lzxpress_round_trip.c | 5 ++++-
4 files changed, 49 insertions(+), 14 deletions(-)
Changeset truncated at 500 lines:
diff --git a/lib/compression/lzxpress_huffman.c b/lib/compression/lzxpress_huffman.c
index 990552a7d09..3eac8e3b2b6 100644
--- a/lib/compression/lzxpress_huffman.c
+++ b/lib/compression/lzxpress_huffman.c
@@ -928,7 +928,7 @@ struct write_context {
size_t head; /* where lengths go */
size_t next_code; /* where symbol stream goes */
size_t pending_next_code; /* will be next_code */
- int bit_len;
+ unsigned bit_len;
uint32_t bits;
};
@@ -953,7 +953,8 @@ static inline bool write_bits(struct write_context *wc,
if (wc->bit_len > 16) {
uint32_t w = wc->bits >> (wc->bit_len - 16);
wc->bit_len -= 16;
- if (wc->next_code + 2 > wc->dest_len) {
+ if (wc->next_code + 2 > wc->dest_len ||
+ unlikely(wc->bit_len > 16)) {
return false;
}
wc->dest[wc->next_code] = w & 0xff;
@@ -969,6 +970,9 @@ static inline bool write_bits(struct write_context *wc,
static inline bool write_code(struct write_context *wc, uint16_t code)
{
int code_bit_len = bitlen_nonzero_16(code);
+ if (unlikely(code == 0)) {
+ return false;
+ }
code &= (1 << code_bit_len) - 1;
return write_bits(wc, code, code_bit_len);
}
@@ -1045,7 +1049,7 @@ static ssize_t write_compressed_bytes(uint16_t symbol_values[512],
}
len = intermediate[i + 1];
- len |= intermediate[i + 2] << 16;
+ len |= intermediate[i + 2] << 16U;
distance = intermediate[i + 3];
i += 3;
} else if (c == 0xffff) {
@@ -1058,6 +1062,9 @@ static ssize_t write_compressed_bytes(uint16_t symbol_values[512],
} else {
return LZXPRESS_ERROR;
}
+ if (unlikely(distance == 0)) {
+ return LZXPRESS_ERROR;
+ }
/* len has already had 3 subtracted */
if (len >= 15) {
/*
@@ -1383,6 +1390,11 @@ static void debug_tree_codes(struct bitstream *input)
int k;
uint16_t j = q.code_code;
size_t offset = bitlen_nonzero_16(j) - 1;
+ if (unlikely(j == 0)) {
+ DBG("BROKEN code is 0!\n");
+ return;
+ }
+
for (k = 0; k <= offset; k++) {
bool b = (j >> (offset - k)) & 1;
bits[k] = b ? '1' : '0';
@@ -1788,17 +1800,19 @@ static ssize_t lzx_huffman_decompress_block(struct bitstream *input,
* the end of the expected block. That's fine, so long
* as it doesn't extend past the total output size.
*/
+ size_t i;
size_t end = output_pos + length;
+ uint8_t *here = output + output_pos;
+ uint8_t *there = here - distance;
if (end > output_size ||
previous_size + output_pos < distance ||
- unlikely(end < output_pos)) {
+ unlikely(end < output_pos || there > here)) {
return LZXPRESS_ERROR;
}
-
- for (; output_pos < end; output_pos++) {
- output[output_pos] = \
- output[output_pos - distance];
+ for (i = 0; i < length; i++) {
+ here[i] = there[i];
}
+ output_pos += length;
distance = 0;
length = 0;
}
diff --git a/lib/compression/tests/test_lzx_huffman.c b/lib/compression/tests/test_lzx_huffman.c
index 3a055183f7b..b7f22b9072b 100644
--- a/lib/compression/tests/test_lzx_huffman.c
+++ b/lib/compression/tests/test_lzx_huffman.c
@@ -344,6 +344,7 @@ static void test_lzxpress_huffman_decompress(void **state)
p.compressed.length,
dest,
p.decompressed.length);
+ assert_int_not_equal(written, -1);
assert_int_equal(written, p.decompressed.length);
assert_memory_equal(dest, p.decompressed.data, p.decompressed.length);
@@ -368,6 +369,7 @@ static void test_lzxpress_huffman_compress(void **state)
p.decompressed.length,
&dest);
+ assert_int_not_equal(written, -1);
assert_int_equal(written, p.compressed.length);
assert_memory_equal(dest, p.compressed.data, p.compressed.length);
talloc_free(dest);
@@ -444,7 +446,8 @@ static void test_lzxpress_huffman_decompress_files(void **state)
dest,
p.decompressed.length);
debug_end_timer("decompress", p.decompressed.length);
- if (written == p.decompressed.length &&
+ if (written != -1 &&
+ written == p.decompressed.length &&
memcmp(dest, p.decompressed.data, p.decompressed.length) == 0) {
debug_message("\033[1;32mdecompressed %s!\033[0m\n", p.name);
score++;
@@ -691,6 +694,7 @@ static void test_lzxpress_huffman_round_trip(void **state)
print_message("total reference size: %zd \n", reference_total);
print_message("diff: %7zd \n",
reference_total - compressed_total);
+ assert_true(reference_total != 0);
print_message("ratio: \033[1;3%dm%.2f\033[0m \n",
2 + (compressed_total >= reference_total),
((double)compressed_total) / reference_total);
diff --git a/lib/compression/tests/test_lzxpress_plain.c b/lib/compression/tests/test_lzxpress_plain.c
index 17e5a26207b..b1a4bee6e92 100644
--- a/lib/compression/tests/test_lzxpress_plain.c
+++ b/lib/compression/tests/test_lzxpress_plain.c
@@ -294,7 +294,8 @@ static void test_lzxpress_plain_decompress_more_compressed_files(void **state)
dest,
p.decompressed.length);
debug_end_timer("decompress", p.decompressed.length);
- if (written == p.decompressed.length &&
+ if (written != -1 &&
+ written == p.decompressed.length &&
memcmp(dest, p.decompressed.data, p.decompressed.length) == 0) {
debug_message("\033[1;32mdecompressed %s!\033[0m\n", p.name);
score++;
@@ -474,6 +475,7 @@ static void test_lzxpress_plain_round_trip_files(void **state)
print_message("total reference size: %zd \n", reference_total);
print_message("diff: %7zd \n",
reference_total - compressed_total);
+ assert_true(reference_total != 0);
print_message("ratio: \033[1;3%dm%.2f\033[0m \n",
2 + (compressed_total >= reference_total),
((double)compressed_total) / reference_total);
@@ -790,7 +792,7 @@ static void test_msft_data1(void **state)
strlen(fixed_data),
out,
talloc_get_size(out));
-
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
out2 = talloc_size(tmp_ctx, strlen(fixed_data));
@@ -798,7 +800,7 @@ static void test_msft_data1(void **state)
sizeof(fixed_out),
out2,
talloc_get_size(out2));
-
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -830,7 +832,7 @@ static void test_msft_data2(void **state)
strlen(fixed_data),
out,
talloc_get_size(out));
-
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
@@ -840,6 +842,7 @@ static void test_msft_data2(void **state)
out2,
talloc_get_size(out2));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -877,6 +880,7 @@ static void test_lzxpress(void **state)
out,
talloc_get_size(out));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
@@ -886,6 +890,7 @@ static void test_lzxpress(void **state)
out2,
talloc_get_size(out2));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -895,6 +900,7 @@ static void test_lzxpress(void **state)
out3,
talloc_get_size(out3));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out3, fixed_data, c_size);
@@ -927,6 +933,7 @@ static void test_lzxpress2(void **state)
out,
talloc_get_size(out));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
@@ -936,6 +943,7 @@ static void test_lzxpress2(void **state)
out2,
talloc_get_size(out2));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -971,6 +979,7 @@ static void test_lzxpress3(void **state)
out,
talloc_get_size(out));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
@@ -980,6 +989,7 @@ static void test_lzxpress3(void **state)
out2,
talloc_get_size(out2));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -1015,6 +1025,7 @@ static void test_lzxpress4(void **state)
out,
talloc_get_size(out));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, sizeof(fixed_out));
assert_memory_equal(out, fixed_out, c_size);
@@ -1024,6 +1035,7 @@ static void test_lzxpress4(void **state)
out2,
talloc_get_size(out2));
+ assert_int_not_equal(c_size, -1);
assert_int_equal(c_size, strlen(fixed_data));
assert_memory_equal(out2, fixed_data, c_size);
@@ -1135,6 +1147,7 @@ static void test_lzxpress_round_trip(void **state)
data,
alloc_size);
+ assert_int_not_equal(len, -1);
assert_int_equal(len, comp.length);
assert_memory_equal(comp.data, data, len);
@@ -1144,6 +1157,7 @@ static void test_lzxpress_round_trip(void **state)
data,
alloc_size);
+ assert_int_not_equal(len, -1);
assert_int_equal(len, uncomp.length);
assert_memory_equal(uncomp.data, data, len);
diff --git a/lib/fuzzing/fuzz_lzxpress_round_trip.c b/lib/fuzzing/fuzz_lzxpress_round_trip.c
index a6173bb68c9..ac38368527e 100644
--- a/lib/fuzzing/fuzz_lzxpress_round_trip.c
+++ b/lib/fuzzing/fuzz_lzxpress_round_trip.c
@@ -27,7 +27,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
int LLVMFuzzerTestOneInput(uint8_t *buf, size_t len)
{
- static uint8_t compressed[1024 * 1024] = {0};
+ static uint8_t compressed[1024 * 1280] = {0};
static uint8_t decompressed[1024 * 1024] = {0};
ssize_t compressed_size;
ssize_t decompressed_size;
@@ -38,6 +38,9 @@ int LLVMFuzzerTestOneInput(uint8_t *buf, size_t len)
compressed_size = lzxpress_compress(buf, len,
compressed, sizeof(compressed));
+ if (compressed_size < 0) {
+ abort();
+ }
decompressed_size = lzxpress_decompress(compressed, compressed_size,
decompressed, sizeof(decompressed));
--
Samba Shared Repository
More information about the samba-cvs
mailing list