[PATCH 0/4] more robust functions for parsing int from buf
From: Jeff King <hidden>
Date: 2025-11-30 13:14:01
On Wed, Nov 26, 2025 at 09:22:38AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:quoted
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.
quoted
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.
quoted
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