From: Phillip Wood Date: Mon, 11 Jul 2022 10:10:20 GMT Subject: Re: [PATCH 4/7] xdiff: use G[C]ALLOC_ARRAY(), not XDL_CALLOC_ARRAY() Message-ID: <37667911-3049-4e78-e67d-9fcb7389dfc0@gmail.com> In-Reply-To: Hi Ævar On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote: > Use the newly created GCALLOC_ARRAY() helpers rather than the recently > introduced XDL_[C]ALLOC_ARRAY(). > > As shown in this diff the calling convention differs, we cannot use > GCALLOC_ARRAY() as an expression, but that's an advantage in not > having to relay the "sizeof()" down via a wrapper function. > > This also: > > * Fixes long-standing potential overflow issues, as we're using > st_mult() in the underlying G_[C]ALLOC(). Note that the What issues is this fixing? XDL_ALLOC_ARRAY() already checks for overflow. > * Slightly optimizes the "XDL_CALLOC_ARRAY", as we'll now use > calloc() rather than malloc() + memset() (although smart compilers > will probably do the same for both...). That's addressed in V2 of my series, unfortunately I sent it just after you'd sent this series. > * Changes the "XDL_CALLOC_ARRAY" behavior where we'd shortcut if the > size was already large enough, but this behavior was changed when > XDL_ALLOC_ARRAY() was introduced, so this is safe. I'm not sure what you mean here - how did we shortcut before? > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > xdiff/xdiffi.c | 3 ++- > xdiff/xhistogram.c | 9 ++++++--- > xdiff/xmacros.h | 12 ------------ > xdiff/xpatience.c | 6 ++++-- > xdiff/xprepare.c | 24 ++++++++++++++++-------- > 5 files changed, 28 insertions(+), 26 deletions(-) > > diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c > index 6fded43e87d..077cc456087 100644 > --- a/xdiff/xdiffi.c > +++ b/xdiff/xdiffi.c > @@ -333,7 +333,8 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > * One is to store the forward path and one to store the backward path. > */ > ndiags = xe->xdf1.nreff + xe->xdf2.nreff + 3; > - if (!XDL_ALLOC_ARRAY(kvd, 2 * ndiags + 2)) > + GALLOC_ARRAY(kvd, 2 * ndiags + 2); > + if (!kvd) > return -1; > kvdf = kvd; > kvdb = kvdf + ndiags; > diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c > index df909004c10..f20592bfbdd 100644 > --- a/xdiff/xhistogram.c > +++ b/xdiff/xhistogram.c > @@ -266,14 +266,17 @@ static int find_lcs(xpparam_t const *xpp, xdfenv_t *env, > > index.table_bits = xdl_hashbits(count1); > index.records_size = 1 << index.table_bits; > - if (!XDL_CALLOC_ARRAY(index.records, index.records_size)) > + GCALLOC_ARRAY(index.records, index.records_size); > + if (!index.records) I don't think that having GALLOC_ARRAY() as a statement is an improvement here. Best Wishes Phillip > goto cleanup; > > index.line_map_size = count1; > - if (!XDL_CALLOC_ARRAY(index.line_map, index.line_map_size)) > + GCALLOC_ARRAY(index.line_map, index.line_map_size); > + if (!index.line_map) > goto cleanup; > > - if (!XDL_CALLOC_ARRAY(index.next_ptrs, index.line_map_size)) > + GCALLOC_ARRAY(index.next_ptrs, index.line_map_size); > + if (!index.next_ptrs) > goto cleanup; > > /* lines / 4 + 1 comes from xprepare.c:xdl_prepare_ctx() */ > diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h > index d13a6724629..75506bdf17e 100644 > --- a/xdiff/xmacros.h > +++ b/xdiff/xmacros.h > @@ -49,18 +49,6 @@ do { \ > ((unsigned long) __p[2]) << 16 | ((unsigned long) __p[3]) << 24; \ > } while (0) > > -/* Allocate an array of nr elements, returns NULL on failure */ > -#define XDL_ALLOC_ARRAY(p, nr) \ > - ((p) = SIZE_MAX / sizeof(*(p)) >= (size_t)(nr) \ > - ? xdl_malloc((nr) * sizeof(*(p))) \ > - : NULL) > - > -/* Allocate an array of nr zeroed out elements, returns NULL on failure */ > -#define XDL_CALLOC_ARRAY(p, nr) \ > - (XDL_ALLOC_ARRAY((p), (nr)) \ > - ? memset((p), 0, (nr) * sizeof(*(p))) \ > - : NULL) > - > /* > * Ensure array p can accommodate at least nr elements, growing the > * array and updating alloc (which is the number of allocated > diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c > index fe39c2978cb..bb328d9f852 100644 > --- a/xdiff/xpatience.c > +++ b/xdiff/xpatience.c > @@ -151,7 +151,8 @@ static int fill_hashmap(mmfile_t *file1, mmfile_t *file2, > > /* We know exactly how large we want the hash map */ > result->alloc = count1 * 2; > - if (!XDL_CALLOC_ARRAY(result->entries, result->alloc)) > + GCALLOC_ARRAY(result->entries, result->alloc); > + if (!result->entries) > return -1; > > /* First, fill with entries from the first file */ > @@ -208,7 +209,8 @@ static int find_longest_common_sequence(struct hashmap *map, struct entry **res) > */ > int anchor_i = -1; > > - if (!XDL_ALLOC_ARRAY(sequence, map->nr)) > + GALLOC_ARRAY(sequence, map->nr); > + if (!sequence) > return -1; > > for (entry = map->first; entry; entry = entry->next) { > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c > index c84549f6c50..d6cbee32a2a 100644 > --- a/xdiff/xprepare.c > +++ b/xdiff/xprepare.c > @@ -78,15 +78,17 @@ static int xdl_init_classifier(xdlclassifier_t *cf, long size, long flags) { > > return -1; > } > - if (!XDL_CALLOC_ARRAY(cf->rchash, cf->hsize)) { > + GCALLOC_ARRAY(cf->rchash, cf->hsize); > + if (!cf->rchash) { > > xdl_cha_free(&cf->ncha); > return -1; > } > > cf->alloc = size; > - if (!XDL_ALLOC_ARRAY(cf->rcrecs, cf->alloc)) { > > + GALLOC_ARRAY(cf->rcrecs, cf->alloc); > + if (!cf->rcrecs) { > xdl_free(cf->rchash); > xdl_cha_free(&cf->ncha); > return -1; > @@ -170,12 +172,14 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ > > if (xdl_cha_init(&xdf->rcha, sizeof(xrecord_t), narec / 4 + 1) < 0) > goto abort; > - if (!XDL_ALLOC_ARRAY(recs, narec)) > + GALLOC_ARRAY(recs, narec); > + if (!recs) > goto abort; > > hbits = xdl_hashbits((unsigned int) narec); > hsize = 1 << hbits; > - if (!XDL_CALLOC_ARRAY(rhash, hsize)) > + GCALLOC_ARRAY(rhash, hsize); > + if (!rhash) > goto abort; > > nrec = 0; > @@ -196,14 +200,17 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ > } > } > > - if (!XDL_CALLOC_ARRAY(rchg, nrec + 2)) > + GCALLOC_ARRAY(rchg, nrec + 2); > + if (!rchg) > goto abort; > > if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) && > (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF)) { > - if (!XDL_ALLOC_ARRAY(rindex, nrec + 1)) > + GALLOC_ARRAY(rindex, nrec + 1); > + if (!rindex) > goto abort; > - if (!XDL_ALLOC_ARRAY(ha, nrec + 1)) > + GALLOC_ARRAY(ha, nrec + 1); > + if (!ha) > goto abort; > } > > @@ -369,7 +376,8 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd > xdlclass_t *rcrec; > char *dis, *dis1, *dis2; > > - if (!XDL_CALLOC_ARRAY(dis, xdf1->nrec + xdf2->nrec + 2)) > + GCALLOC_ARRAY(dis, xdf1->nrec + xdf2->nrec + 2); > + if (!dis) > return -1; > dis1 = dis; > dis2 = dis1 + xdf1->nrec + 1;