Re: [PATCH 6/7] xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc()

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 6/7] xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc()
Date: Fri, 08 Jul 2022 23:44:27 +0200	[thread overview]
Message-ID: <220708.868rp39t28.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <1bf5d574-4202-31e3-4dc3-d7890465d28f@gmail.com>

On Fri, Jul 08 2022, Phillip Wood wrote:

> On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
>> As noted in 36c83197249 (xdiff: use xmalloc/xrealloc, 2019-04-11) the
>> reason we have xdl_malloc() in the first place was to make the xdiff
>> code safer, it was not handling some allocation failures correctly at
>> the time.
>
> This is untrue, we do not have xdl_malloc to make the xdiff code
> safer, that macro was introduced with the initial import of
> xdiff. 36c83197249   just changed its definition, the entire commit
> consists of
>
> -#define xdl_malloc(x) malloc(x)
> +#define xdl_malloc(x) xmalloc(x)
>  #define xdl_free(ptr) free(ptr)
> -#define xdl_realloc(ptr,x) realloc(ptr,x)
> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)

Yes sorry about that, I'm missing a few words there, and meant "the
reason we have this incarnation of xdl_malloc()[...]", or something to
that effect.

> I can see the argument for reverting that change now that we have
> fixed the error checking but that is not a good reason to remove
> xdl_malloc().

Indeed, but hopefully
https://lore.kernel.org/git/220708.86czef9t6y.gmgdl@evledraar.gmail.com/
is that argument.

>> But as noted in that commit doing this was intended as a stopgap, as
>> various code in xdiff/* did not correctly handle allocation failures,
>> and would have segfaulted if malloc() returned NULL.
>> But since then and after preceding commits we can be confident that
>> malloc() failures are handled correctly. All of these users of
>> xdl_malloc() are checking their return values, and we're returning
>> -1 (or similar ) to the top-level in case of failures.
>> This also has the big advantage of making the compiler and analysis
>> tools less confused, and potentially avoiding undefined (compiler)
>> behavior. I.e. we define our own xmalloc() to call die() on failure,
>> and that function uses the non-standard "noreturn" attribute on our
>> most common compiler targets.
>> But xdl_malloc()'s use conflicted with that, confusing both human
>> readers of this code, and potentially compilers as well.
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   xdiff/xdiff.h  |  1 -
>>   xdiff/xdiffi.c |  2 +-
>>   xdiff/xmerge.c | 10 +++++-----
>>   xdiff/xutils.c |  2 +-
>>   4 files changed, 7 insertions(+), 8 deletions(-)
>> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
>> index 832cf9d977e..df048e0099b 100644
>> --- a/xdiff/xdiff.h
>> +++ b/xdiff/xdiff.h
>> @@ -119,7 +119,6 @@ typedef struct s_bdiffparam {
>>   } bdiffparam_t;
>>     
>> -#define xdl_malloc(x) xmalloc(x)
>>   #define xdl_free(ptr) free(ptr)
>>     void *xdl_mmfile_first(mmfile_t *mmf, long *size);
>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
>> index 077cc456087..6590811634f 100644
>> --- a/xdiff/xdiffi.c
>> +++ b/xdiff/xdiffi.c
>> @@ -368,7 +368,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>   static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2) {
>>   	xdchange_t *xch;
>>   -	if (!(xch = (xdchange_t *) xdl_malloc(sizeof(xdchange_t))))
>> +	if (!(xch = (xdchange_t *) malloc(sizeof(xdchange_t))))
>>   		return NULL;
>>     	xch->next = xscr;
>> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
>> index ac0cf52f3be..676ad39020d 100644
>> --- a/xdiff/xmerge.c
>> +++ b/xdiff/xmerge.c
>> @@ -60,7 +60,7 @@ static int xdl_append_merge(xdmerge_t **merge, int mode,
>>   		m->chg1 = i1 + chg1 - m->i1;
>>   		m->chg2 = i2 + chg2 - m->i2;
>>   	} else {
>> -		m = xdl_malloc(sizeof(xdmerge_t));
>> +		m = malloc(sizeof(xdmerge_t));
>>   		if (!m)
>>   			return -1;
>>   		m->next = NULL;
>> @@ -409,7 +409,7 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
>>   		m->i2 = xscr->i2 + i2;
>>   		m->chg2 = xscr->chg2;
>>   		while (xscr->next) {
>> -			xdmerge_t *m2 = xdl_malloc(sizeof(xdmerge_t));
>> +			xdmerge_t *m2 = malloc(sizeof(xdmerge_t));
>>   			if (!m2) {
>>   				xdl_free_env(&xe);
>>   				xdl_free_script(x);
>> @@ -670,7 +670,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
>>   						 ancestor_name,
>>   						 favor, changes, NULL, style,
>>   						 marker_size);
>> -		result->ptr = xdl_malloc(size);
>> +		result->ptr = malloc(size);
>>   		if (!result->ptr) {
>>   			xdl_cleanup_merge(changes);
>>   			return -1;
>> @@ -718,14 +718,14 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
>>   	}
>>   	status = 0;
>>   	if (!xscr1) {
>> -		result->ptr = xdl_malloc(mf2->size);
>> +		result->ptr = malloc(mf2->size);
>>   		if (!result->ptr)
>>   			goto out;
>>   		status = 0;
>>   		memcpy(result->ptr, mf2->ptr, mf2->size);
>>   		result->size = mf2->size;
>>   	} else if (!xscr2) {
>> -		result->ptr = xdl_malloc(mf1->size);
>> +		result->ptr = malloc(mf1->size);
>>   		if (!result->ptr)
>>   			goto out;
>>   		status = 0;
>> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
>> index c0cd5338c4e..865e08f0e93 100644
>> --- a/xdiff/xutils.c
>> +++ b/xdiff/xutils.c
>> @@ -98,7 +98,7 @@ void *xdl_cha_alloc(chastore_t *cha) {
>>   	void *data;
>>     	if (!(ancur = cha->ancur) || ancur->icurr == cha->nsize) {
>> -		if (!(ancur = (chanode_t *) xdl_malloc(sizeof(chanode_t) + cha->nsize))) {
>> +		if (!(ancur = (chanode_t *) malloc(sizeof(chanode_t) + cha->nsize))) {
>>     			return NULL;
>>   		}