[2/3] xdiff: introduce XDL_CALLOC_ARRAY()

Commit Message

Phillip Wood June 29, 2022, 3:25 p.m. UTC

From: Phillip Wood <[e-mail]>

Add a helper for allocating an array and initialize the elements to
zero. This is analogous to CALLOC_ARRAY() in the rest of the codebase
but it returns NULL on allocation failures rather than dying to
accommodate other users of libxdiff such as libgit2.

Signed-off-by: Phillip Wood <[e-mail]>
---
 xdiff/xhistogram.c | 19 ++++++-------------
 xdiff/xmacros.h    |  6 ++++++
 xdiff/xpatience.c  |  5 +----
 xdiff/xprepare.c   | 14 ++++----------
 4 files changed, 17 insertions(+), 27 deletions(-)

Comments

Junio C Hamano June 30, 2022, 6:17 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h
> index 9fd3c5da91a..23db8e785d7 100644
> --- a/xdiff/xmacros.h
> +++ b/xdiff/xmacros.h
> @@ -55,4 +55,10 @@ do { \
>  		? 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)
> +

This implementation is somewhat dissapointing.  Allocating and then
clearing is conceptually different from allocating an already
cleared buffer.

Wouldn't it make more sense to build on top of xcalloc() or its
counterpart in xdl world by introducing xdl_calloc()?  For that,
this step would probably need to become two patches.  The first
patch introduces xdl_calloc(), which uses xcalloc() in our codebase,
and turn the existing "alloc and clear" code to use it, and the
second patch would wrap the use of xdl_calloc() in the size-safe
macro XDL_CALLOC_ARRAY().

Phillip Wood July 6, 2022, 1:17 p.m. UTC | #2

Hi Junio

On 30/06/2022 19:17, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h
>> index 9fd3c5da91a..23db8e785d7 100644
>> --- a/xdiff/xmacros.h
>> +++ b/xdiff/xmacros.h
>> @@ -55,4 +55,10 @@ do { \
>>   		? 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)
>> +
> 
> This implementation is somewhat dissapointing.  Allocating and then
> clearing is conceptually different from allocating an already
> cleared buffer.
> 
> Wouldn't it make more sense to build on top of xcalloc() or its
> counterpart in xdl world by introducing xdl_calloc()?  For that,
> this step would probably need to become two patches.  The first
> patch introduces xdl_calloc(), which uses xcalloc() in our codebase,
> and turn the existing "alloc and clear" code to use it, and the
> second patch would wrap the use of xdl_calloc() in the size-safe
> macro XDL_CALLOC_ARRAY().

I was hoping to avoid that by following the existing pattern of malloc() 
+ memset() but I'll reroll with a preparatory patch that converts the 
existing code to use xdl_calloc()

Best Wishes

Phillip

Patch

diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
index 01decffc332..df909004c10 100644
--- a/xdiff/xhistogram.c
+++ b/xdiff/xhistogram.c
@@ -251,7 +251,7 @@  static int find_lcs(xpparam_t const *xpp, xdfenv_t *env,
 		    int line1, int count1, int line2, int count2)
 {
 	int b_ptr;
-	int sz, ret = -1;
+	int ret = -1;
 	struct histindex index;
 
 	memset(&index, 0, sizeof(index));
@@ -265,23 +265,16 @@  static int find_lcs(xpparam_t const *xpp, xdfenv_t *env,
 	index.rcha.head = NULL;
 
 	index.table_bits = xdl_hashbits(count1);
-	sz = index.records_size = 1 << index.table_bits;
-	sz *= sizeof(struct record *);
-	if (!(index.records = (struct record **) xdl_malloc(sz)))
+	index.records_size = 1 << index.table_bits;
+	if (!XDL_CALLOC_ARRAY(index.records, index.records_size))
 		goto cleanup;
-	memset(index.records, 0, sz);
 
-	sz = index.line_map_size = count1;
-	sz *= sizeof(struct record *);
-	if (!(index.line_map = (struct record **) xdl_malloc(sz)))
+	index.line_map_size = count1;
+	if (!XDL_CALLOC_ARRAY(index.line_map, index.line_map_size))
 		goto cleanup;
-	memset(index.line_map, 0, sz);
 
-	sz = index.line_map_size;
-	sz *= sizeof(unsigned int);
-	if (!(index.next_ptrs = (unsigned int *) xdl_malloc(sz)))
+	if (!XDL_CALLOC_ARRAY(index.next_ptrs, index.line_map_size))
 		goto cleanup;
-	memset(index.next_ptrs, 0, sz);
 
 	/* lines / 4 + 1 comes from xprepare.c:xdl_prepare_ctx() */
 	if (xdl_cha_init(&index.rcha, sizeof(struct record), count1 / 4 + 1) < 0)
diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h
index 9fd3c5da91a..23db8e785d7 100644
--- a/xdiff/xmacros.h
+++ b/xdiff/xmacros.h
@@ -55,4 +55,10 @@  do { \
 		? 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)
+
 #endif /* #if !defined(XMACROS_H) */
diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
index ce87b9084ca..fe39c2978cb 100644
--- a/xdiff/xpatience.c
+++ b/xdiff/xpatience.c
@@ -151,11 +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;
-	result->entries = (struct entry *)
-		xdl_malloc(result->alloc * sizeof(struct entry));
-	if (!result->entries)
+	if (!XDL_CALLOC_ARRAY(result->entries, result->alloc))
 		return -1;
-	memset(result->entries, 0, result->alloc * sizeof(struct entry));
 
 	/* First, fill with entries from the first file */
 	while (count1--)
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index 25866a1667a..b016570c488 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -78,12 +78,11 @@  static int xdl_init_classifier(xdlclassifier_t *cf, long size, long flags) {
 
 		return -1;
 	}
-	if (!(cf->rchash = (xdlclass_t **) xdl_malloc(cf->hsize * sizeof(xdlclass_t *)))) {
+	if (!XDL_CALLOC_ARRAY(cf->rchash, cf->hsize)) {
 
 		xdl_cha_free(&cf->ncha);
 		return -1;
 	}
-	memset(cf->rchash, 0, cf->hsize * sizeof(xdlclass_t *));
 
 	cf->alloc = size;
 	if (!XDL_ALLOC_ARRAY(cf->rcrecs, cf->alloc)) {
@@ -183,9 +182,8 @@  static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
 
 	hbits = xdl_hashbits((unsigned int) narec);
 	hsize = 1 << hbits;
-	if (!(rhash = (xrecord_t **) xdl_malloc(hsize * sizeof(xrecord_t *))))
+	if (!XDL_CALLOC_ARRAY(rhash, hsize))
 		goto abort;
-	memset(rhash, 0, hsize * sizeof(xrecord_t *));
 
 	nrec = 0;
 	if ((cur = blk = xdl_mmfile_first(mf, &bsize))) {
@@ -209,9 +207,8 @@  static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
 		}
 	}
 
-	if (!(rchg = (char *) xdl_malloc((nrec + 2) * sizeof(char))))
+	if (!XDL_CALLOC_ARRAY(rchg, nrec + 2))
 		goto abort;
-	memset(rchg, 0, (nrec + 2) * sizeof(char));
 
 	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
 	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF)) {
@@ -383,11 +380,8 @@  static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
 	xdlclass_t *rcrec;
 	char *dis, *dis1, *dis2;
 
-	if (!(dis = (char *) xdl_malloc(xdf1->nrec + xdf2->nrec + 2))) {
-
+	if (!XDL_CALLOC_ARRAY(dis, xdf1->nrec + xdf2->nrec + 2))
 		return -1;
-	}
-	memset(dis, 0, xdf1->nrec + xdf2->nrec + 2);
 	dis1 = dis;
 	dis2 = dis1 + xdf1->nrec + 1;