Thread (13 messages) 13 messages, 4 authors, 2025-12-05
STALE297d

[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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help