Re: [PATCH v7 10/28] reftable: (de)serialization for the polymorphic record type.
From: Andrzej Hunt <hidden>
Date: 2021-05-04 17:23:33
On 19/04/2021 13:37, Han-Wen Nienhuys via GitGitGadget wrote:
From: Han-Wen Nienhuys <redacted>
[...snip...]
quoted hunk ↗ jump to hunk
diff --git a/reftable/record.c b/reftable/record.c new file mode 100644 index 000000000000..b111852b667e --- /dev/null +++ b/reftable/record.c[...snip...> +static void reftable_obj_record_copy_from(void *rec, const void *src_rec, + int hash_size) +{ + struct reftable_obj_record *obj = rec; + const struct reftable_obj_record *src = + (const struct reftable_obj_record *)src_rec; + int olen; + + reftable_obj_record_release(obj); + *obj = *src; + obj->hash_prefix = reftable_malloc(obj->hash_prefix_len); + memcpy(obj->hash_prefix, src->hash_prefix, obj->hash_prefix_len); + + olen = obj->offset_len * sizeof(uint64_t); + obj->offsets = reftable_malloc(olen); + memcpy(obj->offsets, src->offsets, olen); +}
If src->offset_len is 0 then offsets will be NULL - and passing NULL into memcpy() results in undefined behaviour. I think we should either add an "if (src->offset_len)" check around the memcpy, or perhaps switch to COPY_ARRAY() which performs that check for us. (We can probably also skip the malloc and hence also olen calculation in this scenario though, because obj->offsets should already be NULL if src->offsets was NULL?)
Specifically, if I run t0032 as such:
make CC=clang-11 UBSAN_OPTIONS=print_stacktrace=1 SANITIZE=undefined COPTS="-Og -g" GIT_TEST_OPTS=-v T=t0032-reftable-unittest.sh test
UBSAN reports the following error:
reftable/record.c:475:23: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
#0 0x698e52 in reftable_obj_record_copy_from record.c:475:2
#1 0x6ac806 in test_copy record_test.c:21:2
#2 0x6abb73 in test_reftable_obj_record_roundtrip record_test.c:334:3
#3 0x6abb73 in record_test_main record_test.c:403:2
#4 0x437f08 in cmd__reftable test-reftable.c:10:2
#5 0x427e74 in cmd_main test-tool.c:126:11
#6 0x428043 in main common-main.c:52:11
#7 0x7f3cd1044349 in __libc_start_main (/lib64/libc.so.6+0x24349)
#8 0x407209 in _start start.S:120
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior reftable/record.c:475:23 in
[...snip...]quoted hunk ↗ jump to hunk
+static int reftable_obj_record_decode(void *rec, struct strbuf key, + uint8_t val_type, struct string_view in, + int hash_size) +{ + struct string_view start = in; + struct reftable_obj_record *r = rec; + uint64_t count = val_type; + int n = 0; + uint64_t last; + int j; + r->hash_prefix = reftable_malloc(key.len); + memcpy(r->hash_prefix, key.buf, key.len); + r->hash_prefix_len = key.len; + + if (val_type == 0) { + n = get_var_int(&count, &in); + if (n < 0) { + return n; + } + + string_view_consume(&in, n); + } + + r->offsets = NULL; + r->offset_len = 0; + if (count == 0) + return start.len - in.len;
And it looks like creating a reftable_obj_record with offsets = NULL and offset_len = 0 is considered valid based on the code above. [... snipped the rest ...]