Re: [PATCH] git-compat-util: introduce `count_t` typedef

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 7, 2025, 14:17 UTC
Message-ID: <582e8e75-c6eb-4845-8f3b-62f234f0964f@gmail.com>
In-Reply-To: <20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im>

Hi Patrick

On 07/08/2025 10:22, Patrick Steinhardt wrote:

Show 6 quoted lines

> Historically, Git has been very lenient with its use of integer types
> and didn't really give much thought into which type to use in what
> situation. We interchangeably mix and match signed and unsigned types
> and often times blindly convert them. This use has led to several
> out-of-bounds reads and writes in the past, some of which could be
> turned into arbitrary code execution.

My feeling is that one of the main problems has been using different types for loop indexes and loop limits. If we mandated that the loop index had to be the same type as the limit that would improve things considerably and without mandating a particular type.

Show 9 quoted lines

> A discussion that regularly comes up in this context though is what
> types to use for counting entities:
> 
>    - One question is whether the type should be signed or unsigned.
>      Arguably, the answer should be to use unsigned types as long as we
>      know that we never need a negative value, e.g. as a sentinel. This
>      helps guide the reader and explicitly conveys the sense that such a
>      counter is only ever going to be a non-negative number. Otherwise,
>      code would need to be more careful as it may hold negative values.

The counter argument to this is that it is easy to write incorrect loops when counting down if the loop variable is unsigned. Using a typedef that hides the actual type makes that harder to spot as it is not immediately obvious whether the loop index is signed or not. As we have cases that do need to store a negative value then we're still left with using a mix of signed and unsigned types for counting in our code base.

Show 6 quoted lines

> Introduce a new typedef for `count_t` that is of type `uintptr_t` to
> give clear guidance what type to use for counting entities. This type
> was chosen because in the worst case, an entity may be a single byte and
> we fill all of our memory with these entities. As `uintptr_t` is
> guaranteed to hold at least the value of a pointer, we know that it
> could be used to index into every single such entity.

How many sites actually allocate anything like that number of entities?Generally we use ALLOC_GROW() or ALLOC_GROW_BY() which means that we're not normally counting bytes. ALLOC_GROW_BY() assumes the number of entities fits into a size_t so should be be changing that to use count_t? If we're worried about overflows then maybe we should look at alloc_nr() which calculates the new allocation with

     (nr + 16) * 3 / 2

which which will start overflowing long before we starting allocating UINTPTR_MAX single byte entities.

Thanks

Phillip