Re: [PATCH v7 10/28] reftable: (de)serialization for the polymorphic record type.

From: Andrzej Hunt <andrzej@ahunt.org>
To: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: "Han-Wen Nienhuys" <hanwen@google.com>,
	"Jeff King" <peff@peff.net>,
	"Ramsay Jones" <ramsay@ramsayjones.plus.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Josh Steadmon" <steadmon@google.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Patrick Steinhardt" <ps@pks.im>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Han-Wen Nienhuys" <hanwenn@gmail.com>
Subject: Re: [PATCH v7 10/28] reftable: (de)serialization for the polymorphic record type.
Date: Tue, 4 May 2021 19:23:22 +0200	[thread overview]
Message-ID: <ef664c5c-d959-e265-d678-6fc54d23f031@ahunt.org> (raw)
In-Reply-To: <028dd13e701bf82523cc6da1e68a9dab480c1b27.1618832277.git.gitgitgadget@gmail.com>

On 19/04/2021 13:37, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
[...snip...]
> 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...]
> +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 ...]