Re: [PATCH 21/30] repository: Implement extensions.compatObjectFormat

From: Eric W. Biederman <ebiederm@gmail.com>
Date: Oct 2, 2023, 00:48 UTC
Message-ID: <87r0mdetmv.fsf@gmail.froward.int.ebiederm.org>
In-Reply-To: <xmqqedigizms.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 29 quoted lines

> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> Did you have any manual merge conflicts you had to resolve?
>> If so it is possible to see the merge result you had?
>
> The only merge-fix I had to apply to make everything compile was
> this:
>
> diff --git a/bloom.c b/bloom.c
> index ff131893cd..59eb0a0481 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -278,7 +278,7 @@ static int has_entries_with_high_bit(struct repository *r, struct tree *t)
>  		struct tree_desc desc;
>  		struct name_entry entry;
>  
> -		init_tree_desc(&desc, t->buffer, t->size);
> +		init_tree_desc(&desc, &t->object.oid, t->buffer, t->size);
>  		while (tree_entry(&desc, &entry)) {
>  			size_t i;
>  			for (i = 0; i < entry.pathlen; i++) {
>
> as one topic changed the function signature while the other topic
> added a new callsite.
>
> Everything else was pretty-much auto resolved, I think.
>
> Output from "git show --cc seen" matches my recollection.  The above
> does appear as an evil merge.

Thanks, and I found all of this on your seen branch.

After tracking all of these down it appears all of the errors came from my branch, I will be resending the patches as soon as I finish going through the review comments.

Looking at the build errors pretty much all of the all of the automatic test failures came from commit_tree_extended.

There was a strbuf that did strbuf_init(&buf, 8192); strbuf_init(&buf, 8192); twice.

Plus there was another buffer that was allocated and not freed, in commit_tree_extended.

The leaks were a bit tricky to track down as building with SANITIZE=leak causes tests to fail somewhat randomly for me with "gcc (Debian 12.2.0-14) 12.2.0".

There was one smatch static-analysis error that suggested using CALLOC_ARRAY instead of xcalloc.

The "win" build and "linux-gcc-default (ubuntu-lastest)" build failed because of an over eager gcc warning -Werror=array-bounds. Claiming:

 In file included from /usr/include/string.h:535,
                  from git-compat-util.h:228,
                  from commit.c:1:
 In function ‘memcpy’,
     inlined from ‘oidcpy’ at hash-ll.h:272:2,
     inlined from ‘commit_tree_extended’ at commit.c:1705:3:
 ##[error]/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: ‘__builtin_memcpy’ offset [0, 31] is out of the bounds [0, 0] [-Werror=array-bounds]
    29 |   return __builtin___memcpy_chk (__dest, __src, __len,
       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    30 |                                  __glibc_objsize0 (__dest));
       |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~

Previously in commit_tree_extended the structure was:

	while (parents) {
		struct commit *parent = pop_commit(&parents);
		strbuf_addf(&buffer, "parent %s\n",
			    oid_to_hex(&parent->object.oid));
	}

brian had changed it to:

	nparents = commit_list_count(parents);
	parent_buf = xcalloc(nparents, sizeof(*parent_buf));
	for (i = 0; i < nparents; i++) {
		struct commit *parent = pop_commit(&parents);
		oidcpy(&parent_buf[i], &parent->object.oid);
	}

Which is perfectly sound code.

I changed the structure of the loop to:

	nparents = commit_list_count(parents);
	parent_buf = xcalloc(nparents, sizeof(*parent_buf));
	i = 0;
	while (parents) {
		struct commit *parent = pop_commit(&parents);
		oidcpy(&parent_buf[i++], &parent->object.oid);
	}

And the "array-bounds" warning had no problems with the code. So it looks like the error was actually that array-bounds thought there was a potential NULL pointer dereference at which point it would not have array bounds, and then it complained about the array bounds, instead of the NULL pointer dereference.

I am going to fix the patch. If array-bounds causes further problems you may want to think about disabling it.

Eric