Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()

["René Scharfe" <l.s.r@web.de>]

On 5/14/26 9:07 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Simplify the code by calling st_add3() to do overflow checks instead of
>> open-coding it.  This changes the error message to include the offending
>> summands, which can be helpful when tracking down the cause.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  strbuf.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index 3e04addc22..bb04d3910e 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -106,12 +106,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
>>  void strbuf_grow(struct strbuf *sb, size_t extra)
>>  {
>>  	int new_buf = !sb->alloc;
>> -	if (unsigned_add_overflows(extra, 1) ||
>> -	    unsigned_add_overflows(sb->len, extra + 1))
>> -		die("you want to use way too much memory");
>>  	if (new_buf)
>>  		sb->buf = NULL;
>> -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
>> +	ALLOC_GROW(sb->buf, st_add3(sb->len, extra, 1), sb->alloc);
> 
> ALLOC_GROW() being a macro that references its second argument three
> times, doesn't this rewrite rely on the compiler being clever enough
> to notice that checking for the same overflow three times is
> pointless and does it only once?  I guess the original has the same
> issue already, so this may not be so bad but it makes me feel a bit
> queasy.

As long as it has no side-effect (as is the case for addition) and we
keep compiler optimization enabled we should be fine.

I guess the reason for having multiple references as opposed to
loading the value once into a private variable is to support arbitrary
types.  In the end it needs to fit into a size_t, though.  Something
like this could bring the reference count down to one:

--- 8< ---
diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..ca89cfb0b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -812,6 +812,15 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
+static inline bool st_alloc_nr(size_t nr, size_t alloc, size_t *out)
+{
+	if (nr > alloc) {
+		*out = alloc_nr(alloc) < nr ? nr : alloc_nr(alloc);
+		return true;
+	}
+	return false;
+}
+
 /**
  * Dynamically growing an array using realloc() is error prone and boring.
  *
@@ -857,12 +866,10 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
  */
 #define ALLOC_GROW(x, nr, alloc) \
 	do { \
-		if ((nr) > alloc) { \
-			if (alloc_nr(alloc) < (nr)) \
-				alloc = (nr); \
-			else \
-				alloc = alloc_nr(alloc); \
-			REALLOC_ARRAY(x, alloc); \
+		size_t alloc_grow_new_alloc_; \
+		if (st_alloc_nr((nr), (alloc), &alloc_grow_new_alloc_)) { \
+			alloc = alloc_grow_new_alloc_; \
+			REALLOC_ARRAY(x, alloc_grow_new_alloc_); \
 		} \
 	} while (0)
 
--- >8 ---

Hmm, alloc_nr() doesn't do any overflow checking.  It should, though,
shouldn't it?

René