Re: [RFC PATCH 08/15] strbuf.c: use st_add3(), not unsigned_add_overflows()

From: René Scharfe <l.s.r@web.de>
Date: Jun 4, 2022, 21:27 UTC
Message-ID: <5db7bbf7-a6be-20c9-8233-92fbcb94a266@web.de>
In-Reply-To: <RFC-patch-08.15-2c4b7832144-20220603T183608Z-avarab@gmail.com>

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Show 6 quoted lines

> Change the strbuf_grow() function to use st_add3() instead of doing
> its own unsigned_add_overflows() checks.  The overflow checking here
> was originally added in b449f4cfc97 (Rework strbuf API and semantics.,
> 2007-09-06) and adjusted in 1368f65002b (compat: helper for detecting
> unsigned overflow, 2010-10-10). Instead we compute a "sz" with
> st_add3().

Good idea.

> That was done at a time when the underlying xrealloc() in
> git-compat-util.h didn't use st_mult() yet, that has been the case
> since the later e7792a74bcf (harden REALLOC_ARRAY and xcalloc against
> size_t overflow, 2016-02-22).

How is that relevant?

Show 5 quoted lines

> The only behavior change here should be the very obscure edge case
> that we'd previously die() in cases where we strictly didn't need to,
> as we'd check both "extra + 1" and "sb->len + extra + 1" for
> overflow. If we overflowed only on the latter but were doing the
> former we'd needlessly die() die.

st_add3 does the same checks.  What do you mean with "doing the former"?

Show 24 quoted lines

> I don't think that difference
> mattered, but it's noted here for completeness.
>
> While we're at it add an inline comment about why we're adding 1 to
> the values, that's also explained in the API documentation in
> strbuf.h, but since we're using that magic constant here...
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  strbuf.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 61c4630aeeb..f0a74d2bfb1 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -91,12 +91,12 @@ 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");
> +	const size_t sz_buf = new_buf ? 0 : sb->len;

This is a change in behavior. The one you meant above? It ignores the len value if alloc is zero. Any len value other than zero is invalid in that case. Adding code to paper over this invalid state seems a waste. And if a brave caller fiddled with len before calling strbuf_grow(), the resulting allocation will now be shorter. Hopefully nobody does that, but it's not totally impossible.

We could report the issue instead with something like the following:

	if (new_buf && sb->len)
		BUG("unallocated strbuf has invalid len: %"PRIuMAX, (uintmax_t)sb->len);

I'd rather keep the old behavior. But whatever we decide to do, I think the st_add3 change deserves its own commit with no further change in behavior.

> +	const size_t sz = st_add3(sz_buf, extra, 1 /* for \0 */);

I don't mind the added comment.

Show 8 quoted lines

> +
>  	if (new_buf)
>  		sb->buf = NULL;
> -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> +	ALLOC_GROW(sb->buf, sz, sb->alloc);
>  	if (new_buf && !sb->buf)
>  		BUG("for a new buffer ALLOC_GROW() should always do work!");
>  	if (new_buf)