From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 30 Oct 2026 11:44:40 GMT
Subject: Re: [PATCH v2 4/4] convert: use size_t for text statistics and ident counts
Message-ID: <auSDKKHJONZjx2LX@pks.im>
In-Reply-To: <84e9e38a3e63a2b135847471a13ea36d37bd64d4.1793352943.git.gitter.spiros@gmail.com>

On Fri, Oct 30, 2026 at 10:58:53AM +0100, Elia Pinto wrote:
> gather_stats() counts NULs, CRs, LFs, CRLFs and (non)printable
> characters in "unsigned" fields of struct text_stat, and
> count_ident() counts "$Id$" markers in an "int". Now that the buffer
> sizes they work on are size_t, a large enough buffer overflows them.

This sounds a bit too deterministic: it doesn't necessarily overflow
them, but the possibility exists in theory.

> Unlike the previous patches, this is not specific to LLP64: "unsigned"
> is 32 bits wide on all the platforms we care about.
> 
> The counters only drive decisions, but a wrapped counter makes the
> wrong one. For example, a file with exactly 2^32 lone LFs gets
> lonelf == 0, so will_convert_lf_to_crlf() decides there is nothing
> to convert, and a wrapped "printable" or "nonprintable" can fool the
> heuristic in convert_is_binary(). In count_ident(), going past
> INT_MAX is undefined behaviour.
> 
> This is not a memory safety problem: the counts are only used as
> hints to strbuf_grow() in crlf_to_worktree() and ident_to_worktree(),
> and the buffers are then filled with strbuf_add(), which grows them
> as needed.

These paragraphs very much read like AI to me. I would appreciate a
human touch, also to demonstrate that a human was involved between us
and the AI that tries to understand what was done.

> Use size_t for all of them, and compute the size hint in
> ident_to_worktree() with st_add() and st_mult(), so that it dies
> instead of wrapping around.

That's awfully specific.

The patch itself looks good to me, thanks.

Patrick