Re: [PATCH] strvec: use size_t to store nr and alloc
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-11 17:03:23
On Sat, Sep 11 2021, Jeff King wrote:
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 <redacted> --- 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/ (local) 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/ (local) 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...