All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: Patrick Steinhardt <ps@pks.im>,
	 git@vger.kernel.org,
	 Oswald Buddenhagen <oswald.buddenhagen@gmx.de>,
	 Taylor Blau <me@ttaylorr.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH] git-compat-util: introduce `count_t` typedef
Date: Thu, 07 Aug 2025 09:43:25 -0700	[thread overview]
Message-ID: <xmqq4iujqdzm.fsf@gitster.g> (raw)
In-Reply-To: <582e8e75-c6eb-4845-8f3b-62f234f0964f@gmail.com> (Phillip Wood's message of "Thu, 7 Aug 2025 15:17:14 +0100")

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Patrick
>
> On 07/08/2025 10:22, Patrick Steinhardt wrote:
>> 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.

Yup.  And the limit being unsigned would force the counter to be
also unsigned, which can introduce buggy constructs (like counting
down).

>> 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.

This is very valid argument against typedef for something trivial
like an integer.  Use of proposed count_t loses both size and
signedness information.

Thanks.

  reply	other threads:[~2025-08-07 16:43 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-07  9:22 [PATCH] git-compat-util: introduce `count_t` typedef Patrick Steinhardt
2025-08-07 11:00 ` Matthias Aßhauer
2025-08-07 14:17 ` Phillip Wood
2025-08-07 16:43   ` Junio C Hamano [this message]
2025-08-07 16:38 ` Junio C Hamano
2025-08-07 22:07   ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4iujqdzm.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=oswald.buddenhagen@gmx.de \
    --cc=peff@peff.net \
    --cc=phillip.wood123@gmail.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.