On 11/09/2021 17:13, Ævar Arnfjörð Bjarmason wrote: > 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 <peff@xxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxxxx/ >> >> 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@xxxxxxxxxxxxxxxxxxx/ 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... I'm particularly interested in the int -> size_t change problem as part of the wider 4GB limitations for the LLP64 systems [0] such as the RaspPi, git-lfs (on windows [1]), and Git-for-Windows[2]. It is a big problem. Philip [0] http://nickdesaulniers.github.io/blog/2016/05/30/data-models-and-word-size/ [1] https://github.com/git-lfs/git-lfs/issues/2434 ; Git on Windows client corrupts files > 4Gb [2] https://github.com/git-for-windows/git/pull/2179 ; [DRAFT] for testing : Fix 4Gb limit for large files on Git for Windows
