Re: [PATCH 6/7] xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc()
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 8, 2022, 17:42 UTC
- Message-ID
- <1bf5d574-4202-31e3-4dc3-d7890465d28f@gmail.com>
- In-Reply-To
- <patch-6.7-33d93f121a9-20220708T140354Z-avarab@gmail.com>
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
Show 112 quoted lines
> 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;
> }