Re: [PATCH] strvec: use size_t to store nr and alloc

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 11, 2021, 16:13 UTC
Message-ID: <87o88z82pc.fsf@evledraar.gmail.com>
In-Reply-To: <YTzEvLHWcfuD20x4@coredump.intra.peff.net>

On Sat, Sep 11 2021, Jeff King wrote:

Show 26 quoted lines

> We converted argv_array (which later became strvec) to use size_t in
> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
> order to avoid the possibility of integer overflow. But later, commit
> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
> converted these back to ints!
>
> Those two commits were part of the same patch series. I'm pretty sure
> what happened is that they were originally written in the opposite order
> and then cleaned up and re-ordered during an interactive rebase. And
> when resolving the inevitable conflict, I mistakenly took the "rename"
> patch completely, accidentally dropping the type change.
>
> We can correct it now; better late than never.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This was posted previously in the midst of another thread, but I don't
> think was picked up. There was some positive reaction, but one "do we
> really need this?" to which I responded in detail:
>
>   https://lore.kernel.org/git/YTIBnT8Ue1HZXs82@coredump.intra.peff.net/
>
> I don't really think any of that needs to go into the commit message,
> but if that's a hold-up, I can try to summarize it (though I think
> referring to the commit which _already_ did this and was accidentally
> reverted would be sufficient).

Thanks, I have a WIP version of this outstanding starting with this patch that I was planning to submit sometime, but I'm happy to have you pursue it, especially with the ~100 outstanding patches I have in master..seen.

It does feel somewhere between iffy and a landmine waiting to be stepped on to only convert the member itself, and not any of the corresponding "int" variables that track it to "size_t".

If you do the change I suggested in https://lore.kernel.org/git/87v93i8svd.fsf@evledraar.gmail.com/ you'll find that there's at least one first-order reference to this that now uses "int" that if converted to "size_t" will result in a wrap-around error, we're lucky that one has a test failure.

I can tell you what that bug is, but maybe it's better if you find it yourself :) I.e. I found *that* one, but I'm not sure I found them all. I just s/int nr/size_t *nr/ and eyeballed the wall off compiler errors & the code context (note: pointer, obviously broken, but makes the compiler yell).

That particular bug will be caught by the compiler as it involves a >= 0 comparison against unsigned, but we may not not have that everywhere...