From: Phillip Wood Date: Fri, 08 Jul 2022 17:42:17 GMT Subject: Re: [PATCH 6/7] xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc() Message-ID: <1bf5d574-4202-31e3-4dc3-d7890465d28f@gmail.com> In-Reply-To: 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) 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(). Best Wishes Phillip > 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 > --- > 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; > }