From: Jeff King Date: Sun, 30 Nov 2025 13:13:51 GMT Subject: [PATCH 0/4] more robust functions for parsing int from buf Message-ID: <20251130131351.GA198697@coredump.intra.peff.net> In-Reply-To: On Wed, Nov 26, 2025 at 09:22:38AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Hmm, I thought both of those things were reasonably clever. The other > > obvious way to do it, AFAICT, is to used checked-operation intrinsics or > > add unsigned_add_overflows() before every operation. > > Yup, but the thing is, I didn't want something "clever". I prefer > "clean and obvious" if we add extra code for safety. Yeah, that's fair. It turns out that one half of that is easy: checking for overflow as we compute the number). And one half is hard. If you don't assume a twos-complement style range where the "min = -max - 1", then you are stuck using INT_MIN. Which is OK for "int", but not for arbitrary types. We already make the same assumption in git_parse_int(), etc. So I went with that approach here, but it is at least documented clearly. > > It looks like you merged what I had into 'next'. Where do you want to go > > from there? I am mostly content to let it be, but we can also try to > > replace with something like your version. > > That is my preference. While the topic is still in 'next', or after > the topic graduates to 'master'. Either is fine. And it is fine if > such an update did not come, too. After all, this is to deal with > contents in a locally generated file (.git/index), so a maliciously > corrupt string that lack the expected whitespace character after the > digit string is a sign that you are trying to burn yourself and you > have only yourself to blame, isn't it? An attacker that can put > garbage in your .git/index has better ways to fool you by updating > your .git/config file that sits next to it. Or teach the sanitizer > that this code path is already OK somehow? Yeah, I agree the stakes are low here. Though they were somewhat low to begin with for the same reason! But I was grossed out enough by the whole thing that I tried to put together a decent helper for parsing integers from buffers, and converted both sites here. I suspect it could be used in other places, too, but I didn't convert any. > > Or even, I guess, work on a > > global strntoi() that could be used everywhere, if we think it is robust > > enough. (Though technically that name is reserved by the standard, which > > is a shame, because that is really what this thing is). > > Well, we already use plenty of names beginning with 'str' followed > by a lowercase letter, like strbuf_foo() and string_list_init(). In the end it was sufficiently different from strtoi() that I decided not to use that name. It was but one of many bike-sheddable decisions, which I tried to document. So I guess let the flaming commence. ;) This is built on top of jk/asan-bonanza. [1/4]: parse: prefer bool to int for boolean returns [2/4]: parse: add functions for parsing from non-string buffers [3/4]: cache-tree: use parse_int_from_buf() [4/4]: fsck: use parse_unsigned_from_buf() for parsing timestamp Makefile | 1 + cache-tree.c | 28 ++----- compat/posix.h | 2 + fsck.c | 20 +---- parse.c | 162 +++++++++++++++++++++++++++++-------- parse.h | 31 +++++-- t/meson.build | 1 + t/unit-tests/u-parse-int.c | 98 ++++++++++++++++++++++ 8 files changed, 263 insertions(+), 80 deletions(-) create mode 100644 t/unit-tests/u-parse-int.c -Peff