{"thread":{"id":"64468","subject":"[PATCH 0/9] asan bonanza","startedAt":"2025-11-12T07:55:24Z","lastAt":"2026-01-21T05:27:51Z","messageCount":64,"participants":["Jeff King","Collin Funk","Patrick Steinhardt","Junio C Hamano","Taylor Blau","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"530567","messageId":"20251112075522.GA978866@coredump.intra.peff.net","threadId":"64468","inReplyTo":null,"subject":"[PATCH 0/9] asan bonanza","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T07:55:22Z","receivedAt":"2025-11-12T07:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a handful of issues that ASan finds in our test suite\nif we tweak a few options to let it look deeper.\n\nThe cache-tree one was reported to the security list. It's a real bug,\nbut I don't think is an interesting vulnerability (it's a benign read\noff the end of an mmap'd file that is local and not generally under\nattacker control).\n\nThe bitmap bug is also a real bug in new code that I think is not well\nexercised yet (+cc Taylor for that one).\n\nThe fsck changes are for false positives in ASan, but I think it is\nreasonable for it to complain about this sketchy code. ;) I hope the\nresult is nicer to read and reason about, but whether it is worth the\nchurn may be debatable.\n\nAlong the way we can turn a few knobs that will potentially help us find\nmore problems down the road (but ordered so that \"make SANITIZE=address\"\npasses at each step of the series).\n\n [1/9]: compat/mmap: mark unused argument in git_munmap()\n [2/9]: pack-bitmap: handle name-hash lookups in incremental bitmaps\n [3/9]: Makefile: turn on NO_MMAP when building with ASan\n [4/9]: cache-tree: avoid strtol() on non-string buffer\n [5/9]: fsck: assert newline presence in fsck_ident()\n [6/9]: fsck: avoid strcspn() in fsck_ident()\n [7/9]: fsck: remove redundant date timestamp check\n [8/9]: fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated\n [9/9]: t: enable ASan's strict_string_checks option\n\n Makefile | 1 +\n cache-tree.c | 45 ++++++++++++++++++++++----------\n compat/mmap.c | 2 +-\n fsck.c | 71 ++++++++++++++++++++++++++++++++++++---------------\n pack-bitmap.c | 27 +++++++++++++++++---\n t/test-lib.sh | 1 +\n 6 files changed, 107 insertions(+), 40 deletions(-)\n\n-Peff\n"},{"id":"530568","messageId":"20251112075652.GA979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 1/9] compat/mmap: mark unused argument in git_munmap()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T07:56:52Z","receivedAt":"2025-11-12T07:56:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our mmap compat code emulates mapping by using malloc/free. Our\ngit_munmap() must take a \"length\" parameter to match the interface of\nmunmap(), but we don't use it (it is up to the allocator to know how big\nthe block is in free()).\n\nLet's mark it as UNUSED to avoid complaints from -Wunused-parameter.\nOtherwise you cannot build with \"make DEVELOPER=1 NO_MMAP=1\".\n\nSigned-off-by: Jeff King \n---\nThis made me wonder if nobody is using NO_MMAP at all. But it may just\nbe that platforms which need it are not using -Werror in the first\nplace.\n\n compat/mmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex 2fe1c7732e..1a118711f7 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -38,7 +38,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \treturn start;\n }\n \n-int git_munmap(void *start, size_t length)\n+int git_munmap(void *start, size_t length UNUSED)\n {\n \tfree(start);\n \treturn 0;\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530569","messageId":"20251112080151.GB979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:01:51Z","receivedAt":"2025-11-12T08:01:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a bitmap has a name-hash cache, it is an array of 32-bit integers,\none per entry in the bitmap, which we've mmap'd from the .bitmap file.\nWe access it directly like this:\n\n if (bitmap_git->hashes)\n hash = get_be32(bitmap_git->hashes + index_pos);\n\nThat works for both regular pack bitmaps and for non-incremental midx\nbitmaps. There is one bitmap_index with one \"hashes\" array, and\nindex_pos is within its bounds (we do the bounds-checking when we load\nthe bitmap).\n\nBut for an incremental midx bitmap, we have a linked list of\nbitmap_index structs, and each one has only its own small slice of the\nname-hash array. If index_pos refers to an object that is not in the\nfirst bitmap_git of the chain, then we'll access memory outside of the\nbounds of its \"hashes\" array, and often outside of the mmap.\n\nInstead, we should walk through the list until we find the bitmap_index\nwhich serves our index_pos, and use its hash (after adjusting index_pos\nto make it relative to the slice we found). This is exactly what we do\nelsewhere for incremental midx lookups (like the pack_pos_to_midx() call\na few lines above). But we can't use existing helpers like\nmidx_for_object() here, because we're walking through the chain of\nbitmap_index structs (each of which refers to a midx), not the chain of\nincremental multi_pack_index structs themselves.\n\nThe problem is triggered in the test suite, but we don't get a segfault\nbecause the out-of-bounds index is too small. The OS typically rounds\nour mmap up to the nearest page size, so we just end up accessing some\nextra zero'd memory. Nor do we catch it with ASan, since it doesn't seem\nto instrument mmaps at all. But if we build with NO_MMAP, then our maps\nare replaced with heap allocations, which ASan does check. And so:\n\n make NO_MMAP=1 SANITIZE=address\n cd t\n ./t5334-incremental-multi-pack-index.sh\n\ndoes show the problem (and this patch makes it go away).\n\nSigned-off-by: Jeff King \n---\nAs always with the midx and bitmap code, I am left unsure of which\nordering it is correct to use (pseudo-pack order, or lexical oid order,\nor how each splits across incremental files). I _think_ this is right\nbecause it's matching the ordering that is already used for a single\nmidx. But clearly this area is under-tested, since even when we did not\ngo off the end of the array we were probably passing back junk\nname-hashes (either from the .bitmap file's trailing checksum, or\nzero-padding at the end of the mapped page).\n\nSo it might be worth adding more tests here, but I know this incremental\nbitmap code is a big work in progress. So I contented myself with the\nreproduction above, and anything else can go onto the incremental todo\npile. :)\n\n pack-bitmap.c | 27 +++++++++++++++++++++++----\n 1 file changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 291e1a9cf4..710b86a451 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -213,6 +213,26 @@ static uint32_t bitmap_num_objects(struct bitmap_index *index)\n \treturn index->pack->num_objects;\n }\n \n+static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n+{\n+\tif (bitmap_is_midx(index)) {\n+\t\twhile (index && pos < index->midx->num_objects_in_base)\n+\t\t\tindex = index->base;\n+\n+\t\tif (!index)\n+\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n+\n+\t\tpos -= index->midx->num_objects_in_base;\n+\t\tif (pos >= index->midx->num_objects)\n+\t\t\tBUG(\"out-of-bounds midx bitmap object at %\"PRIu32, pos);\n+\t}\n+\n+\tif (!index->hashes)\n+\t\treturn 0;\n+\n+\treturn get_be32(index->hashes + pos);\n+}\n+\n static struct repository *bitmap_repo(struct bitmap_index *bitmap_git)\n {\n \tif (bitmap_is_midx(bitmap_git))\n@@ -1724,8 +1744,7 @@ static void show_objects_for_type(\n \t\t\t\tpack = bitmap_git->pack;\n \t\t\t}\n \n-\t\t\tif (bitmap_git->hashes)\n-\t\t\t\thash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t\thash = bitmap_name_hash(bitmap_git, index_pos);\n \n \t\t\tshow_reach(&oid, object_type, 0, hash, pack, ofs, payload);\n \t\t}\n@@ -3124,8 +3143,8 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \n \t\tif (oe) {\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\n-\t\t\tif (bitmap_git->hashes && !oe->hash)\n-\t\t\t\toe->hash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t\tif (!oe->hash)\n+\t\t\t\toe->hash = bitmap_name_hash(bitmap_git, index_pos);\n \t\t}\n \t}\n \n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530570","messageId":"20251112080215.GC979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:02:15Z","receivedAt":"2025-11-12T08:02:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git often uses mmap() to access on-disk files. This leaves a blind spot\nin our SANITIZE=address builds, since ASan does not seem to handle mmap\nat all. Nor does the OS notice most out-of-bounds access, since it tends\nto round up to the nearest page size (so depending on how big the map\nis, you might have to overrun it by up to 4095 bytes to trigger a\nsegfault).\n\nThe previous commit demonstrates a memory bug that we missed. We could\nhave made a new test where the out-of-bounds access was much larger, or\nwhere the mapped file ended closer to a page boundary. But the point of\nrunning the test suite with sanitizers is to catch these problems\nwithout having to construct specific tests.\n\nLet's enable NO_MMAP for our ASan builds by default, which should give\nus better coverage. This does increase the memory usage of Git, since\nwe're copying from the filesystem into heap. But the repositories in the\ntest suite tend to be small, so the overhead isn't really noticeable\n(and ASan already has quite a performance penalty).\n\nThere are a few other known bugs that this patch will help flush out.\nHowever, they aren't directly triggered in the test suite (yet). So\nit's safe to turn this on now without breaking the test suite, which\nwill help us add new tests to demonstrate those other bugs as we fix\nthem.\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 7e0f77e298..0f44268405 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n endif\n ifneq ($(filter address,$(SANITIZERS)),)\n NO_REGEX = NeededForASAN\n+NO_MMAP = NeededForASAN\n SANITIZE_ADDRESS = YesCompiledWithIt\n endif\n endif\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530571","messageId":"20251112080537.GD979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:05:37Z","receivedAt":"2025-11-12T08:05:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A cache-tree extension entry in the index looks like this:\n\n NUL SPACE NEWLINE \n\nwhere the \"_nr\" items are human-readable base-10 ASCII. We parse them\nwith strtol(), even though we do not have a NUL-terminated string (we'd\ngenerally have an mmap() of the on-disk index file). For a well-formed\nentry, this is not a problem; strtol() will stop when it sees the\nnewline. But there are two problems:\n\n 1. A corrupted entry could omit the newline, causing us to read\n further. You'd mostly get stopped by seeing non-digits in the oid\n field (and if it is likewise truncated, there will still be 20 or\n more bytes of the index checksum). So it's possible, though\n unlikely, to see read off the end of the mmap'd buffer. Of course a\n malicious index file can fake the oid and the index checksum to all\n (ASCII) 0's.\n\n This is further complicated by the fact that mmap'd buffers tend to\n be zero-padded up to the page boundary. So to run off the end, the\n index size also has to be a multiple of the page size. This is also\n unlikely, though you can construct a malicious index file that\n matches this.\n\n The security implications aren't too interesting. The index file is\n a local file anyway (so you can't attack somebody by cloning, but\n only if you convince them to operate in a .git directory you made,\n at which point attacking .git/config is much easier). And it's just\n a read overflow via strtol(), which is unlikely to buy you much\n beyond a crash.\n\n 2. ASan has a strict_string_checks option, which tells it to make sure\n that options to string functions (like strtol) have some eventual\n NUL, without regard to what the function would actually do (like\n stopping at a newline here). This option sometimes has false\n positives, but it can point to sketchy areas (like this one) where\n the input we use doesn't exhibit a problem, but different input\n _could_ cause us to misbehave.\n\nLet's fix it by just parsing the values ourselves with a helper function\nthat is careful not to go past the end of the buffer. There are a few\nbehavior changes here that should not matter:\n\n - We do not consider overflow, as strtol() would. But nor did the\n original code. However, we don't trust the value we get from the\n on-disk file, and if it says to read 2^30 entries, we would notice\n that we do not have that many and bail before reading off the end of\n the buffer.\n\n - Our helper does not skip past extra leading whitespace as strtol()\n would, but according to gitformat-index(5) there should not be any.\n\n - The original quit parsing at a newline or a NUL byte, but now we\n insist on a newline (which is what the documentation says, and what\n Git has always produced).\n\nSince we are providing our own helper function, we can tweak the\ninterface a bit to make our lives easier. The original code does not use\nstrtol's \"end\" pointer to find the end of the parsed data, but rather\nuses a separate loop to advance our \"buf\" pointer to the trailing\nnewline. We can instead provide a helper that advances \"buf\" as it\nparses, letting us read strictly left-to-right through the buffer.\n\nI didn't add a new test here. It's surprisingly difficult to construct\nan index of exactly the right size due to the way we pad entries. But it\nis easy to trigger the problem in existing tests when using ASan's\nstrict string checking, coupled with a recent change to use NO_MMAP with\nASan builds. So:\n\n make SANITIZE=address\n cd t\n ASAN_OPTIONS=strict_string_checks=1 ./t0090-cache-tree.sh\n\ntriggers it reliably. Technically it is not deterministic because there\nis ~8% chance (it's 1-(255/256)^20, or ^32 for sha256) that the trailing\nchecksum hash has a NUL byte in it. But we compute enough cache-trees in\nthe course of that script that we are very likely to hit the problem in\none of them.\n\nWe can look at making strict_string_checks the default for ASan builds,\nbut there are some other cases we'd want to fix first.\n\nReported-by: correctmost \nSigned-off-by: Jeff King \n---\nIt feels gross to reimplement strtol(), just because there are so many\nweird corner cases (signs, leading whitespace, overflow, and so on).\nThe alternative is reading into a buffer, NUL-terminating it, and\ncalling strtol() there. But that has its own hazards (you have to decide\nwhen to stop reading, which means reimplementing those same rules to\nsoak up whitespace, etc).\n\nWe can take some shortcuts here because we know what our one caller\nlooks like. It might be worth having a carefully written \"strntol()\"\nthat can be used everywhere. But there aren't _that_ many call sites\nthat would use it, so maybe this ad-hoc approach is better?\n\n cache-tree.c | 45 +++++++++++++++++++++++++++++++--------------\n 1 file changed, 31 insertions(+), 14 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2aba47060e..ab20ffe863 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -548,12 +548,36 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n \ttrace2_region_leave(\"cache_tree\", \"write\", the_repository);\n }\n \n+static long parse_long(const char **ptr, unsigned long *len_p)\n+{\n+\tconst char *s = *ptr;\n+\tunsigned long len = *len_p;\n+\tlong ret = 0;\n+\tint sign = 1;\n+\n+\twhile (len && *s == '-') {\n+\t\tsign *= -1;\n+\t\ts++;\n+\t\tlen--;\n+\t}\n+\n+\twhile (len) {\n+\t\tif (!isdigit(*s))\n+\t\t\tbreak;\n+\t\tret *= 10;\n+\t\tret += *s - '0';\n+\t\ts++;\n+\t\tlen--;\n+\t}\n+\t*ptr = s;\n+\t*len_p = len;\n+\treturn sign * ret;\n+}\n+\n static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)\n {\n \tconst char *buf = *buffer;\n \tunsigned long size = *size_p;\n-\tconst char *cp;\n-\tchar *ep;\n \tstruct cache_tree *it;\n \tint i, subtree_nr;\n \tconst unsigned rawsz = the_hash_algo->rawsz;\n@@ -569,19 +593,12 @@ static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)\n \tbuf++; size--;\n \tit = cache_tree();\n \n-\tcp = buf;\n-\tit->entry_count = strtol(cp, &ep, 10);\n-\tif (cp == ep)\n+\tit->entry_count = parse_long(&buf, &size);\n+\tif (!size || *buf != ' ')\n \t\tgoto free_return;\n-\tcp = ep;\n-\tsubtree_nr = strtol(cp, &ep, 10);\n-\tif (cp == ep)\n-\t\tgoto free_return;\n-\twhile (size && *buf && *buf != '\\n') {\n-\t\tsize--;\n-\t\tbuf++;\n-\t}\n-\tif (!size)\n+\tbuf++; size--;\n+\tsubtree_nr = parse_long(&buf, &size);\n+\tif (!size || *buf != '\\n')\n \t\tgoto free_return;\n \tbuf++; size--;\n \tif (0 <= it->entry_count) {\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530572","messageId":"20251112080609.GE979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 5/9] fsck: assert newline presence in fsck_ident()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:06:09Z","receivedAt":"2025-11-12T08:06:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fsck code purports to handle buffers that are not NUL-terminated,\nbut fsck_ident() uses some string functions. This works OK in practice,\nas explained in 8e4309038f (fsck: do not assume NUL-termination of\nbuffers, 2023-01-19). Before calling fsck_ident() we'll have called\nverify_headers(), which makes sure we have at least a trailing newline.\nAnd none of our string-like functions will walk past that newline.\n\nHowever, that makes this code at the top of fsck_ident() very confusing:\n\n *ident = strchrnul(*ident, '\\n');\n if (**ident == '\\n')\n (*ident)++;\n\nWe should always see that newline, or our memory safety assumptions have\nbeen violated! Further, using strchrnul() is weird, since the whole\npoint is that if the newline is not there, we don't necessarily have a\nNUL at all, and might read off the end of the buffer.\n\nSo let's have callers pass in the boundary of our buffer, which lets us\nsafely find the newline with memchr(). And if it is not there, this is a\nBUG(), because it means our caller did not validate the input with\nverify_headers() as it was supposed to (and we are better off bailing\nrather than having memory-safety problems).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 341e100d24..8991f04943 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -860,16 +860,18 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n-static int fsck_ident(const char **ident,\n+static int fsck_ident(const char **ident, const char *ident_end,\n \t\t const struct object_id *oid, enum object_type type,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n+\tconst char *nl;\n \tchar *end;\n \n-\t*ident = strchrnul(*ident, '\\n');\n-\tif (**ident == '\\n')\n-\t\t(*ident)++;\n+\tnl = memchr(p, '\\n', ident_end - p);\n+\tif (!nl)\n+\t\tBUG(\"verify_headers() should have made sure we have a newline\");\n+\t*ident = nl + 1;\n \n \tif (*p == '<')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n@@ -958,7 +960,7 @@ static int fsck_commit(const struct object_id *oid,\n \tauthor_count = 0;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\terr = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options);\n \t\tif (err)\n \t\t\treturn err;\n \t}\n@@ -970,7 +972,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\treturn err;\n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\terr = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n@@ -1065,7 +1067,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t\t\tgoto done;\n \t}\n \telse\n-\t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n+\t\tret = fsck_ident(&buffer, buffer_end, oid, OBJ_TAG, options);\n \n \tif (buffer < buffer_end && (skip_prefix(buffer, \"gpgsig \", &buffer) || skip_prefix(buffer, \"gpgsig-sha256 \", &buffer))) {\n \t\teol = memchr(buffer, '\\n', buffer_end - buffer);\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530573","messageId":"20251112080639.GF979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 6/9] fsck: avoid strcspn() in fsck_ident()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:06:39Z","receivedAt":"2025-11-12T08:06:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may be operating on a buffer that is not NUL-terminated, but we use\nstrcspn() to parse it. This is OK in practice, as discussed in\n8e4309038f (fsck: do not assume NUL-termination of buffers, 2023-01-19),\nbecause we know there is at least a trailing newline in our buffer, and\nwe always pass \"\\n\" to strcspn(). So we know it will stop before running\noff the end of the buffer.\n\nBut this is a subtle point to hang our memory safety hat on. And it\nconfuses ASan's strict_string_checks mode, even though it is technically\na false positive (that mode complains that we have no NUL, which is\ntrue, but it does not know that we have verified the presence of the\nnewline already).\n\nLet's instead open-code the loop. As a bonus, this makes the logic more\nobvious (to my mind, anyway). The current code skips forward with\nstrcspn until it hits \"<\", \">\", or \"\\n\". But then it must check which it\nsaw to decide if that was what we expected or not, duplicating some\nlogic between what's in the strcspn() and what's in the domain logic.\nInstead, we can just check each character as we loop and act on it\nimmediately.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 32 ++++++++++++++++++++++----------\n 1 file changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 8991f04943..2ee72d573d 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -875,18 +875,30 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \n \tif (*p == '<')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n-\tp += strcspn(p, \"<>\\n\");\n-\tif (*p == '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n-\tif (*p != '<')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\tfor (;;) {\n+\t\tif (p >= ident_end || *p == '\\n')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\t\tif (*p == '>')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n+\t\tif (*p == '<')\n+\t\t\tbreak; /* end of name, beginning of email */\n+\n+\t\t/* otherwise, skip past arbitrary name char */\n+\t\tp++;\n+\t}\n \tif (p[-1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n-\tp++;\n-\tp += strcspn(p, \"<>\\n\");\n-\tif (*p != '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n-\tp++;\n+\tp++; /* skip past '<' we found */\n+\tfor (;;) {\n+\t\tif (p >= ident_end || *p == '<' || *p == '\\n')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n+\t\tif (*p == '>')\n+\t\t\tbreak; /* end of email */\n+\n+\t\t/* otherwise, skip past arbitrary email char */\n+\t\tp++;\n+\t}\n+\tp++; /* skip past '>' we found */\n \tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530574","messageId":"20251112080644.GG979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 7/9] fsck: remove redundant date timestamp check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:06:44Z","receivedAt":"2025-11-12T08:06:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After calling \"parse_timestamp(p, &end, 10)\", we complain if \"p == end\",\nwhich would imply that we did not see any digits at all. But we know\nthis cannot be the case, since we would have bailed already if we did\nnot see any digits, courtesy of extra checks added by 8e4309038f (fsck:\ndo not assume NUL-termination of buffers, 2023-01-19). Since then,\nchecking \"p == end\" is redundant and we can drop it.\n\nThis will make our lives a little easier as we refactor further.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2ee72d573d..266c965cec 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -920,7 +920,7 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n-\tif ((end == p || *end != ' '))\n+\tif (*end != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n \tp = end + 1;\n \tif ((*p != '+' && *p != '-') ||\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530575","messageId":"20251112081040.GH979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:10:40Z","receivedAt":"2025-11-12T08:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In fsck_ident(), we parse the timestamp with parse_timestamp(), which is\nreally an alias for strtoumax(). But since our buffer may not be\nNUL-terminated, this can trigger a complaint from ASan's\nstrict_string_checks mode. This is a false positive, since we know that\nthe buffer contains a trailing newline (which we checked earlier in the\nfunction), and that strtoumax() would stop there.\n\nBut it is worth working around ASan's complaint. One is because that\nwill let us turn on strict_string_checks by default, which has helped\ncatch other real problems. And two is that the safety of the current\ncode is very hard to reason about (it subtly depends on distant code\nwhich could change).\n\nOne option here is to just parse the number left-to-right ourselves. But\nwe care about the size of a timestamp_t and detecting overflow, since\nthat's part of the point of these checks. And doing that correctly is\ntricky. So we'll instead just pull the digits into a separate,\nNUL-terminated buffer, and use that to call parse_timestamp().\n\nSigned-off-by: Jeff King \n---\nThere's one step we could take after this commit, which is to annotate\nall of the spots that look at \"p\" to do:\n\n -if (*p != ' ')\n +if (p >= ident_end || *p != ' ')\n\treturn report(..., \"expected space\");\n\nor similar. And then I think it would be safe to call fsck_ident() with\na buffer that is not NUL-terminated (and does not have our \"safety\"\nnewline). I stopped short for this series because we are just trying to\nappease ASan (and not fixing real bugs), and because the result gets\nrather unwieldy. But it might be worth it in the long run. We could\nalways do it later on top.\n\nAgain, I find the strto*() wrapping to be gross. Here we use the\nextra-buffer trick. But we still don't get away with avoiding custom\nlogic (for example, if we ever want to support negative timestamps, the\nfsck code will have to recognize \"-\" signs). But it feels like the best\nwe can do for now.\n\n fsck.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 266c965cec..8e8083e7c6 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -860,13 +860,28 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n+static timestamp_t parse_timestamp_from_buf(const char **start, const char *end)\n+{\n+\tconst char *p = *start;\n+\tchar buf[24]; /* big enough for 2^64 */\n+\tsize_t i = 0;\n+\n+\twhile (p < end && isdigit(*p)) {\n+\t\tif (i >= ARRAY_SIZE(buf) - 1)\n+\t\t\treturn TIME_MAX;\n+\t\tbuf[i++] = *p++;\n+\t}\n+\tbuf[i] = '\\0';\n+\t*start = p;\n+\treturn parse_timestamp(buf, NULL, 10);\n+}\n+\n static int fsck_ident(const char **ident, const char *ident_end,\n \t\t const struct object_id *oid, enum object_type type,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n \tconst char *nl;\n-\tchar *end;\n \n \tnl = memchr(p, '\\n', ident_end - p);\n \tif (!nl)\n@@ -918,11 +933,11 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \t\t\t \"invalid author/committer line - bad date\");\n \tif (*p == '0' && p[1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n-\tif (date_overflows(parse_timestamp(p, &end, 10)))\n+\tif (date_overflows(parse_timestamp_from_buf(&p, ident_end)))\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n-\tif (*end != ' ')\n+\tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n-\tp = end + 1;\n+\tp++;\n \tif ((*p != '+' && *p != '-') ||\n \t !isdigit(p[1]) ||\n \t !isdigit(p[2]) ||\n-- \n2.52.0.rc1.260.g3e4993586f\n\n"},{"id":"530576","messageId":"20251112081055.GI979063@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH 9/9] t: enable ASan's strict_string_checks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T08:10:55Z","receivedAt":"2025-11-12T08:10:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"ASan has an option to enable strict string checking, where any pointer\npassed to a function that expects a NUL-terminated string will be\nchecked for that NUL termination. This can sometimes produce false\npositives. E.g., it is not wrong to pass a buffer with { '1', '2', '\\n' }\ninto strtoul(). Even though it is not NUL-terminated, it will stop at\nthe newline.\n\nBut in trying it out, it identified two problematic spots in our test\nsuite (which have now been adjusted):\n\n 1. The strtol() parsing in cache-tree.c was a real potential problem,\n which would have been very hard to find otherwise (since it\n required constructing a very specific broken index file).\n\n 2. The use of string functions in fsck_ident() were false positives,\n because we knew that there was always a trailing newline which\n would stop the functions from reading off the end of the buffer.\n But the reasoning behind that is somewhat fragile, and silencing\n those complaints made the code easier to reason about.\n\nSo even though this did not find any earth-shattering bugs, and even had\na few false positives, I'm sufficiently convinced that its complaints\nare more helpful than hurtful. Let's turn it on by default (since the\ntest suite now runs cleanly with it) and see if it ever turns up any\nother instances.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ef0ab7ec2d..0fb76f7d11 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -77,6 +77,7 @@ prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"\n # want that one to complain to stderr).\n prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS\n prepend_var ASAN_OPTIONS : detect_leaks=0\n+prepend_var ASAN_OPTIONS : strict_string_checks=1\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n-- \n2.52.0.rc1.260.g3e4993586f\n"},{"id":"530577","messageId":"87y0obis17.fsf@gmail.com","threadId":"64468","inReplyTo":"20251112080215.GC979063@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-11-12T08:17:24Z","receivedAt":"2025-11-12T08:17:26Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Jeff King writes:\n\n> Git often uses mmap() to access on-disk files. This leaves a blind spot\n> in our SANITIZE=address builds, since ASan does not seem to handle mmap\n> at all. Nor does the OS notice most out-of-bounds access, since it tends\n> to round up to the nearest page size (so depending on how big the map\n> is, you might have to overrun it by up to 4095 bytes to trigger a\n> segfault).\n>\n> The previous commit demonstrates a memory bug that we missed. We could\n> have made a new test where the out-of-bounds access was much larger, or\n> where the mapped file ended closer to a page boundary. But the point of\n> running the test suite with sanitizers is to catch these problems\n> without having to construct specific tests.\n>\n> Let's enable NO_MMAP for our ASan builds by default, which should give\n> us better coverage. This does increase the memory usage of Git, since\n> we're copying from the filesystem into heap. But the repositories in the\n> test suite tend to be small, so the overhead isn't really noticeable\n> (and ASan already has quite a performance penalty).\n>\n> There are a few other known bugs that this patch will help flush out.\n> However, they aren't directly triggered in the test suite (yet). So\n> it's safe to turn this on now without breaking the test suite, which\n> will help us add new tests to demonstrate those other bugs as we fix\n> them.\n>\n> Signed-off-by: Jeff King \n\nI see that an interceptor was added in 2023 [1]. Maybe your compiler is\nolder than that?\n\nOn my system:\n\n $ cat main.c \n #include \n #include \n #include \n int\n main (void)\n {\n char *ptr = mmap (NULL, getpagesize (), PROT_READ | PROT_WRITE,\n \t\t MAP_ANONYMOUS, -1, 0);\n if (ptr == NULL)\n abort ();\n ptr[getpagesize () + 1] = 'a';\n return 0;\n }\n $ gcc --version | head -n 1\n gcc (GCC) 15.2.1 20251022 (Red Hat 15.2.1-3)\n $ clang --version | head -n 1\n clang version 21.1.4 (Fedora 21.1.4-1.fc43)\n $ gcc -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x400554) (BuildId: 1b7a82189bfffb3f73d420e138b9859add25901a) in main\n $ clang -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x4e9ee6) (BuildId: aca1d168eacebaa239082d8a45ab74c8470f4b31) in main\n\n\nCollin\n\n[1] https://github.com/llvm/llvm-project/commit/a34e702aa16fde4cc76e9360d985a64e008e0b23\n"},{"id":"530580","messageId":"20251112103158.GA983233@coredump.intra.peff.net","threadId":"64468","inReplyTo":"87y0obis17.fsf@gmail.com","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-12T10:31:58Z","receivedAt":"2025-11-12T10:32:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 12:17:24AM -0800, Collin Funk wrote:\n\n> I see that an interceptor was added in 2023 [1]. Maybe your compiler is\n> older than that?\n\nNo, I'm using gcc 15.2.0 (from Debian unstable).\n\nBut I'm not sure if the linked code does anything useful for us.\n\nOne, it's not clear to me if it is even kicking in or not. It only does\nanything if the region is \"sanitizer managed\", according to the details\nat https://reviews.llvm.org/D154659. I'm not sure what that means\nexactly, because I'm fuzzy on how the shadow map works.\n\nBut even when it does do something, it seems to round up to the nearest\npage size. But we really want to know if we go even one byte over the\nrequested length, because if we touch the 1235th byte of a 1234-byte\nbuffer (which is going to be a NUL because of mmap rounding up the\npages), then there's probably another test case somewhere where we\naccess the 4097th byte of a 4096-byte buffer (which is going to\nsegfault).\n\n> char *ptr = mmap (NULL, getpagesize (), PROT_READ | PROT_WRITE,\n> \t\t MAP_ANONYMOUS, -1, 0);\n> if (ptr == NULL)\n> abort ();\n\nI think you want to check for MAP_FAILED here, not NULL. And I think we\nalways get that, because MAP_ANONYMOUS needs to be OR-ed into MAP_SHARED\nor MAP_PRIVATE. So here:\n\n> $ gcc -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n> SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x400554) (BuildId: 1b7a82189bfffb3f73d420e138b9859add25901a) in main\n> $ clang -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n> SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x4e9ee6) (BuildId: aca1d168eacebaa239082d8a45ab74c8470f4b31) in main\n\nI don't think this is ASan finding a problem. It is just telling us that\nwe segfaulted for other reasons. And the fault here is because the\nbroken mmap() invocation returned MAP_FAILED, and we tried to access\nthat garbage pointer.\n\n> ptr[getpagesize () + 1] = 'a';\n\nThis is also making a map that is a multiple of the page size, and then\ntouching a byte that's on the next page. That's the easy-ish case that\nwe can often already find, even without ASan (though it depends on what\ncomes after the mapped memory; it might be a valid page).\n\nA more interesting test for Git is to actually map a file, like:\n\n $ cat main.c\n #include \n #include \n #include \n #include \n #include \n static void die(const char *msg)\n {\n \tperror(msg);\n \texit(1);\n }\n int main (int argc, const char **argv)\n {\n \tstruct stat st;\n \tint fd;\n \tchar *ptr;\n \n \tfd = open(argv[1], O_RDONLY);\n \tif (fd < 0)\n \t\tdie(\"open\");\n \tif (fstat(fd, &st) < 0)\n \t\tdie(\"fstat\");\n \tptr = mmap (NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);\n \tif (ptr == MAP_FAILED)\n \t\tdie(\"mmap\");\n \tprintf(\"last byte: %d\\n\", ptr[st.st_size-1]);\n \tprintf(\"one byte after: %d\\n\", ptr[st.st_size]);\n \treturn 0;\n }\n $ yes | head -c 4096 >big\n $ yes | head -c 372 >small\n\nAnd ASan does often detect the problem for the \"big\" page-sized file,\nbut not consistently! If I do:\n\n gcc -fsanitize=address main.c\n while ./a.out big; do echo ok; done\n\nI may get output like:\n\n last byte: 10\n one byte after: 127\n ok\n last byte: 10\n one byte after: 0\n ok\n last byte: 10\n one byte after: 0\n ok\n last byte: 10\n =================================================================\n ==988617==ERROR: AddressSanitizer: unknown-crash on address 0x7efd40b9f000 at pc 0x564fe77b64eb bp 0x7ffff49e8160 sp 0x7ffff49e8158\n READ of size 1 at 0x7efd40b9f000 thread T0\n #0 0x564fe77b64ea in main (/home/peff/a.out+0x14ea) (BuildId: 8db121bb5c048cb336f8be729e8cefebd6f059a3)\n #1 0x7efd41233ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n #2 0x7efd41233d64 in __libc_start_main_impl ../csu/libc-start.c:360\n #3 0x564fe77b6150 in _start (/home/peff/a.out+0x1150) (BuildId: 8db121bb5c048cb336f8be729e8cefebd6f059a3)\n \n Address 0x7efd40b9f000 is a wild pointer inside of access range of size 0x000000000001.\n\nSo it worked three times without ASan noticing the problem (producing\ntwo different outputs), and then ASan finally crashed. But it didn't\ngive us the usual information we get for a malloc overflow. It's just an\n\"unknown crash\" from a \"wild pointer\". So I'm not sure if it's even\nfinding these through its own poisoning, and not just catching an\nunlucky segfault.\n\nIf we switch to the small file, then ASan never reports anything! The OS\ngives us a page-sized chunk, so we consistently read a \"0\" in from the\nbyte after our requested size.\n\nIf we swap out the mmap for:\n\n ptr = malloc(st.st_size);\n read(fd, ptr, st.st_size);\n\n(which is roughly what our NO_MMAP wrapper is doing behind the scenes),\nthen ASan does catch it consistently, even for the \"small\" file:\n\n $ ./a.out small\n =================================================================\n ==1008630==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c7d11fe01b4 at pc 0x55cf89b1b4c8 bp 0x7fff471b84e0 sp 0x7fff471b84d8\n READ of size 1 at 0x7c7d11fe01b4 thread T0\n #0 0x55cf89b1b4c7 in main (/home/peff/a.out+0x14c7) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n #1 0x7f4d12e33ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n #2 0x7f4d12e33d64 in __libc_start_main_impl ../csu/libc-start.c:360\n #3 0x55cf89b1b150 in _start (/home/peff/a.out+0x1150) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n \n 0x7c7d11fe01b4 is located 0 bytes after 372-byte region [0x7c7d11fe0040,0x7c7d11fe01b4)\n allocated by thread T0 here:\n #0 0x7f4d1311a0ab in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67\n #1 0x55cf89b1b3a0 in main (/home/peff/a.out+0x13a0) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n #2 0x7f4d12e33ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\n-Peff\n"},{"id":"530581","messageId":"aRRuvQmlDMBiZolK@pks.im","threadId":"64468","inReplyTo":"20251112080151.GB979063@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-12T11:25:49Z","receivedAt":"2025-11-12T11:26:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 12, 2025 at 03:01:51AM -0500, Jeff King wrote:\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 291e1a9cf4..710b86a451 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -213,6 +213,26 @@ static uint32_t bitmap_num_objects(struct bitmap_index *index)\n> \treturn index->pack->num_objects;\n> }\n> \n> +static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n> +{\n> +\tif (bitmap_is_midx(index)) {\n> +\t\twhile (index && pos < index->midx->num_objects_in_base)\n> +\t\t\tindex = index->base;\n\nSo we first find the MIDX that is supposed to contain the position.\n\n> +\t\tif (!index)\n> +\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n> +\n> +\t\tpos -= index->midx->num_objects_in_base;\n\nWe then subtract the number of objects from all of our predeceding\nlayers from the position. This should result in the position relative to\nthe current layer.\n\n> +\t\tif (pos >= index->midx->num_objects)\n> +\t\t\tBUG(\"out-of-bounds midx bitmap object at %\"PRIu32, pos);\n> +\t}\n> +\n> +\tif (!index->hashes)\n> +\t\treturn 0;\n> +\n> +\treturn get_be32(index->hashes + pos);\n\nAnd we then return the value at that given position. Makes sense to me.\n\nPatrick\n"},{"id":"530582","messageId":"aRRux2uBfORc214r@pks.im","threadId":"64468","inReplyTo":"20251112081040.GH979063@coredump.intra.peff.net","subject":"Re: [PATCH 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-12T11:25:59Z","receivedAt":"2025-11-12T11:26:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 12, 2025 at 03:10:40AM -0500, Jeff King wrote:\n> In fsck_ident(), we parse the timestamp with parse_timestamp(), which is\n> really an alias for strtoumax(). But since our buffer may not be\n> NUL-terminated, this can trigger a complaint from ASan's\n> strict_string_checks mode. This is a false positive, since we know that\n> the buffer contains a trailing newline (which we checked earlier in the\n> function), and that strtoumax() would stop there.\n> \n> But it is worth working around ASan's complaint. One is because that\n> will let us turn on strict_string_checks by default, which has helped\n> catch other real problems. And two is that the safety of the current\n> code is very hard to reason about (it subtly depends on distant code\n> which could change).\n> \n> One option here is to just parse the number left-to-right ourselves. But\n> we care about the size of a timestamp_t and detecting overflow, since\n> that's part of the point of these checks. And doing that correctly is\n> tricky. So we'll instead just pull the digits into a separate,\n> NUL-terminated buffer, and use that to call parse_timestamp().\n\nSo this is another site that would benefit from having something like\n`git_parse_int()` with an extra parameter indicating the number of\nbytes available for parsing (and a way to disable unit factors).\n\nPatrick\n"},{"id":"530583","messageId":"aRRuzrmbJBW8q4Dd@pks.im","threadId":"64468","inReplyTo":"20251112080537.GD979063@coredump.intra.peff.net","subject":"Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-12T11:26:06Z","receivedAt":"2025-11-12T11:26:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 12, 2025 at 03:05:37AM -0500, Jeff King wrote:\n> A cache-tree extension entry in the index looks like this:\n> \n> NUL SPACE NEWLINE \n> \n> where the \"_nr\" items are human-readable base-10 ASCII. We parse them\n> with strtol(), even though we do not have a NUL-terminated string (we'd\n> generally have an mmap() of the on-disk index file). For a well-formed\n> entry, this is not a problem; strtol() will stop when it sees the\n> newline. But there are two problems:\n> \n> 1. A corrupted entry could omit the newline, causing us to read\n> further. You'd mostly get stopped by seeing non-digits in the oid\n> field (and if it is likewise truncated, there will still be 20 or\n> more bytes of the index checksum). So it's possible, though\n> unlikely, to see read off the end of the mmap'd buffer. Of course a\n\ns/see read/read/\n\n> malicious index file can fake the oid and the index checksum to all\n> (ASCII) 0's.\n> \n> This is further complicated by the fact that mmap'd buffers tend to\n> be zero-padded up to the page boundary. So to run off the end, the\n> index size also has to be a multiple of the page size. This is also\n> unlikely, though you can construct a malicious index file that\n> matches this.\n> \n> The security implications aren't too interesting. The index file is\n> a local file anyway (so you can't attack somebody by cloning, but\n> only if you convince them to operate in a .git directory you made,\n> at which point attacking .git/config is much easier). And it's just\n> a read overflow via strtol(), which is unlikely to buy you much\n> beyond a crash.\n\nAgreed. Good to fix it regardless.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 2aba47060e..ab20ffe863 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -548,12 +548,36 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n> \ttrace2_region_leave(\"cache_tree\", \"write\", the_repository);\n> }\n> \n> +static long parse_long(const char **ptr, unsigned long *len_p)\n> +{\n> +\tconst char *s = *ptr;\n> +\tunsigned long len = *len_p;\n> +\tlong ret = 0;\n> +\tint sign = 1;\n> +\n> +\twhile (len && *s == '-') {\n> +\t\tsign *= -1;\n> +\t\ts++;\n> +\t\tlen--;\n> +\t}\n> +\n> +\twhile (len) {\n> +\t\tif (!isdigit(*s))\n> +\t\t\tbreak;\n> +\t\tret *= 10;\n> +\t\tret += *s - '0';\n> +\t\ts++;\n> +\t\tlen--;\n> +\t}\n> +\t*ptr = s;\n> +\t*len_p = len;\n> +\treturn sign * ret;\n> +}\n\nHm. I'm not a huge fan of not having any error handling at all. It just\nfeels way too fragile for my taste:\n\n - As you mention we don't detect overflows, as we would detect them at\n a later point in time when trying to access index entries at invalid\n offsets. But if the input is crafted in a way that the overflow ends\n up with a reasonable index entry we might just as well _not_ detect\n that an overflow has happened and end up using the wrong index\n entry.\n\n - We don't verify that we even have a number in the first place. We'd\n simply return \"0\" in that case and not advance the pointer. This is\n fine though as we verify that the returned size is non-zero, so we'd\n detect this case.\n\nI'd much rather prefer to have an interface similar to `git_parse_int()`\nand related functions, which are way easier to use compared to the likes\nof `stroi()`.\n\nOtherwise, the next time we want to have something similar like you\nintroduce here we might not be aware of this existing helper and may not\nknow to generalize it, so we'd introduce another ad-hoc helper.\n\nAnyway. I won't object if you want to go with your version anyway, as it\nat least fixes one class of errors.\n\nThanks!\n\nPatrick\n"},{"id":"530584","messageId":"aRRu1cxpIzd60AoU@pks.im","threadId":"64468","inReplyTo":"20251112080215.GC979063@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-12T11:26:13Z","receivedAt":"2025-11-12T11:26:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 12, 2025 at 03:02:15AM -0500, Jeff King wrote:\n> diff --git a/Makefile b/Makefile\n> index 7e0f77e298..0f44268405 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n> endif\n> ifneq ($(filter address,$(SANITIZERS)),)\n> NO_REGEX = NeededForASAN\n> +NO_MMAP = NeededForASAN\n> SANITIZE_ADDRESS = YesCompiledWithIt\n> endif\n> endif\n\nLet's also apply this to Meson. Thanks!\n\nPatrick\n\ndiff --git a/meson.build b/meson.build\nindex ad4eb2c4fa..668f8769d2 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1408,12 +1408,18 @@ if host_machine.system() == 'windows'\n libgit_c_args += '-DUSE_WIN32_MMAP'\n else\n checkfuncs += {\n- 'mmap' : ['mmap.c'],\n # provided by compat/mingw.c.\n 'unsetenv' : ['unsetenv.c'],\n # provided by compat/mingw.c.\n 'getpagesize' : [],\n }\n+\n+ if get_option('b_sanitize').contains('address')\n+ libgit_c_args += '-DNO_MMAP'\n+ libgit_sources += 'compat/mmap.c'\n+ else\n+ checkfuncs += { 'mmap': ['mmap.c'] }\n+ endif\n endif\n \n foreach func, impls : checkfuncs\n"},{"id":"530613","messageId":"xmqq346jqc0e.fsf@gitster.g","threadId":"64468","inReplyTo":"aRRux2uBfORc214r@pks.im","subject":"Re: [PATCH 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-12T19:36:17Z","receivedAt":"2025-11-12T19:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> So this is another site that would benefit from having something like\n> `git_parse_int()` with an extra parameter indicating the number of\n> bytes available for parsing (and a way to disable unit factors).\n\nIt is certainly an interesting approach that would work well.\n"},{"id":"530615","messageId":"87qzu32ey0.fsf@gmail.com","threadId":"64468","inReplyTo":"20251112103158.GA983233@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-11-12T20:06:47Z","receivedAt":"2025-11-12T20:06:49Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Jeff King writes:\n\n> On Wed, Nov 12, 2025 at 12:17:24AM -0800, Collin Funk wrote:\n>\n>> I see that an interceptor was added in 2023 [1]. Maybe your compiler is\n>> older than that?\n>\n> No, I'm using gcc 15.2.0 (from Debian unstable).\n>\n> But I'm not sure if the linked code does anything useful for us.\n>\n> One, it's not clear to me if it is even kicking in or not. It only does\n> anything if the region is \"sanitizer managed\", according to the details\n> at https://reviews.llvm.org/D154659. I'm not sure what that means\n> exactly, because I'm fuzzy on how the shadow map works.\n>\n> But even when it does do something, it seems to round up to the nearest\n> page size. But we really want to know if we go even one byte over the\n> requested length, because if we touch the 1235th byte of a 1234-byte\n> buffer (which is going to be a NUL because of mmap rounding up the\n> pages), then there's probably another test case somewhere where we\n> access the 4097th byte of a 4096-byte buffer (which is going to\n> segfault).\n\nThe glibc docs say that the length is rounded up to the nearest page\nsize [1]:\n\n Thus, addresses for mapping must be page-aligned, and length values\n will be rounded up.\n\nThis wording in POSIX makes me think that all systems will round up to\nthe nearest page size [2]:\n\n Thus, while the parameter len need not meet a size or alignment\n constraint, the system shall include, in any mapping operation, any\n partial page specified by the address range starting at pa and\n continuing for len bytes.\n\n>> char *ptr = mmap (NULL, getpagesize (), PROT_READ | PROT_WRITE,\n>> \t\t MAP_ANONYMOUS, -1, 0);\n>> if (ptr == NULL)\n>> abort ();\n>\n> I think you want to check for MAP_FAILED here, not NULL. And I think we\n> always get that, because MAP_ANONYMOUS needs to be OR-ed into MAP_SHARED\n> or MAP_PRIVATE. So here:\n>\n>> $ gcc -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n>> SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x400554) (BuildId: 1b7a82189bfffb3f73d420e138b9859add25901a) in main\n>> $ clang -fsanitize=address main.c && ./a.out 2>&1 | grep ^SUMMARY:\n>> SUMMARY: AddressSanitizer: SEGV (/home/collin/a.out+0x4e9ee6) (BuildId: aca1d168eacebaa239082d8a45ab74c8470f4b31) in main\n>\n> I don't think this is ASan finding a problem. It is just telling us that\n> we segfaulted for other reasons. And the fault here is because the\n> broken mmap() invocation returned MAP_FAILED, and we tried to access\n> that garbage pointer.\n>\n>> ptr[getpagesize () + 1] = 'a';\n>\n> This is also making a map that is a multiple of the page size, and then\n> touching a byte that's on the next page. That's the easy-ish case that\n> we can often already find, even without ASan (though it depends on what\n> comes after the mapped memory; it might be a valid page).\n\nOops. I clearly don't use mmap much. :)\n\n> A more interesting test for Git is to actually map a file, like:\n>\n> $ cat main.c\n> #include \n> #include \n> #include \n> #include \n> #include \n> static void die(const char *msg)\n> {\n> \tperror(msg);\n> \texit(1);\n> }\n> int main (int argc, const char **argv)\n> {\n> \tstruct stat st;\n> \tint fd;\n> \tchar *ptr;\n> \n> \tfd = open(argv[1], O_RDONLY);\n> \tif (fd < 0)\n> \t\tdie(\"open\");\n> \tif (fstat(fd, &st) < 0)\n> \t\tdie(\"fstat\");\n> \tptr = mmap (NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);\n> \tif (ptr == MAP_FAILED)\n> \t\tdie(\"mmap\");\n> \tprintf(\"last byte: %d\\n\", ptr[st.st_size-1]);\n> \tprintf(\"one byte after: %d\\n\", ptr[st.st_size]);\n> \treturn 0;\n> }\n> $ yes | head -c 4096 >big\n> $ yes | head -c 372 >small\n>\n> And ASan does often detect the problem for the \"big\" page-sized file,\n> but not consistently! If I do:\n>\n> gcc -fsanitize=address main.c\n> while ./a.out big; do echo ok; done\n>\n> I may get output like:\n>\n> last byte: 10\n> one byte after: 127\n> ok\n> last byte: 10\n> one byte after: 0\n> ok\n> last byte: 10\n> one byte after: 0\n> ok\n> last byte: 10\n> =================================================================\n> ==988617==ERROR: AddressSanitizer: unknown-crash on address 0x7efd40b9f000 at pc 0x564fe77b64eb bp 0x7ffff49e8160 sp 0x7ffff49e8158\n> READ of size 1 at 0x7efd40b9f000 thread T0\n> #0 0x564fe77b64ea in main (/home/peff/a.out+0x14ea) (BuildId: 8db121bb5c048cb336f8be729e8cefebd6f059a3)\n> #1 0x7efd41233ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n> #2 0x7efd41233d64 in __libc_start_main_impl ../csu/libc-start.c:360\n> #3 0x564fe77b6150 in _start (/home/peff/a.out+0x1150) (BuildId: 8db121bb5c048cb336f8be729e8cefebd6f059a3)\n> \n> Address 0x7efd40b9f000 is a wild pointer inside of access range of size 0x000000000001.\n>\n> So it worked three times without ASan noticing the problem (producing\n> two different outputs), and then ASan finally crashed. But it didn't\n> give us the usual information we get for a malloc overflow. It's just an\n> \"unknown crash\" from a \"wild pointer\". So I'm not sure if it's even\n> finding these through its own poisoning, and not just catching an\n> unlucky segfault.\n>\n> If we switch to the small file, then ASan never reports anything! The OS\n> gives us a page-sized chunk, so we consistently read a \"0\" in from the\n> byte after our requested size.\n>\n> If we swap out the mmap for:\n>\n> ptr = malloc(st.st_size);\n> read(fd, ptr, st.st_size);\n>\n> (which is roughly what our NO_MMAP wrapper is doing behind the scenes),\n> then ASan does catch it consistently, even for the \"small\" file:\n>\n> $ ./a.out small\n> =================================================================\n> ==1008630==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c7d11fe01b4 at pc 0x55cf89b1b4c8 bp 0x7fff471b84e0 sp 0x7fff471b84d8\n> READ of size 1 at 0x7c7d11fe01b4 thread T0\n> #0 0x55cf89b1b4c7 in main (/home/peff/a.out+0x14c7) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n> #1 0x7f4d12e33ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n> #2 0x7f4d12e33d64 in __libc_start_main_impl ../csu/libc-start.c:360\n> #3 0x55cf89b1b150 in _start (/home/peff/a.out+0x1150) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n> \n> 0x7c7d11fe01b4 is located 0 bytes after 372-byte region [0x7c7d11fe0040,0x7c7d11fe01b4)\n> allocated by thread T0 here:\n> #0 0x7f4d1311a0ab in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67\n> #1 0x55cf89b1b3a0 in main (/home/peff/a.out+0x13a0) (BuildId: 2cebfcd0a00064eaaed750af010fcecdae2f5666)\n> #2 0x7f4d12e33ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\nCool, thanks for the actual working example. Your patch makes perfect\nsense now.\n\nCollin\n\n[1] https://www.gnu.org/software/libc/manual/html_node/Memory_002dmapped-I_002fO.html\n[2] https://pubs.opengroup.org/onlinepubs/9799919799/functions/mmap.html\n"},{"id":"530634","messageId":"aRVIh9R8Pnuk+yS0@nand.local","threadId":"64468","inReplyTo":"20251112080151.GB979063@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-11-13T02:55:03Z","receivedAt":"2025-11-13T02:55:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 12, 2025 at 03:01:51AM -0500, Jeff King wrote:\n> As always with the midx and bitmap code, I am left unsure of which\n> ordering it is correct to use (pseudo-pack order, or lexical oid order,\n> or how each splits across incremental files). I _think_ this is right\n> because it's matching the ordering that is already used for a single\n> midx. But clearly this area is under-tested, since even when we did not\n> go off the end of the array we were probably passing back junk\n> name-hashes (either from the .bitmap file's trailing checksum, or\n> zero-padding at the end of the mapped page).\n\nYeah, this is the right order. \"index_pos\" is a good hint that this is\nin lexical order. bitmap_writer_finish() has some oid_pos() lookups that\nuse index directly without sorting, so bitmap_writer_finish() expects\nthis array in lexical order.\n\nCommit c528e17966 (pack-bitmap: write multi-pack bitmaps, 2021-08-31)\nhas a comment in (what is now) midx-write.c explaining this assumption\nin bitmap_writer_finish(), but it should probably be documented\nexplicitly in pack-bitmap.h.\n\n> So it might be worth adding more tests here, but I know this incremental\n> bitmap code is a big work in progress. So I contented myself with the\n> reproduction above, and anything else can go onto the incremental todo\n> pile. :)\n\nYeah, I agree. The only hash-cache test that I could think of is from\nt5326, which tests that we can propagate existing name-hash values from\na pack bitmap in to a MIDX one. We probably need an equivalent for when\nwriting an incremental MIDX/bitmap too. #leftoverbits\n\n> pack-bitmap.c | 27 +++++++++++++++++++++++----\n> 1 file changed, 23 insertions(+), 4 deletions(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 291e1a9cf4..710b86a451 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -213,6 +213,26 @@ static uint32_t bitmap_num_objects(struct bitmap_index *index)\n> \treturn index->pack->num_objects;\n> }\n>\n> +static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n> +{\n> +\tif (bitmap_is_midx(index)) {\n> +\t\twhile (index && pos < index->midx->num_objects_in_base)\n> +\t\t\tindex = index->base;\n\nLooks good. It's too bad that we have to reimplement something very\nsimilar to midx_for_object(), but I agree with what you wrote in the\npatch message and this faithfully captures that. It might be worth doing\nsomething like:\n\n while (index && pos < index->midx->num_objects_in_base) {\n ASSERT(bitmap_is_midx(index));\n index = index->base;\n }\n\n, which should never trigger, but is a good sanity check. Definitely not\nworth re-rolling IMHO.\n\n> +\n> +\t\tif (!index)\n> +\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n> +\n> +\t\tpos -= index->midx->num_objects_in_base;\n> +\t\tif (pos >= index->midx->num_objects)\n> +\t\t\tBUG(\"out-of-bounds midx bitmap object at %\"PRIu32, pos);\n\nmidx_for_object() spells this portion slightly differently, but what you\nhave here is still good.\n\n> +\t}\n> +\n> +\tif (!index->hashes)\n> +\t\treturn 0;\n> +\n> +\treturn get_be32(index->hashes + pos);\n\nWe *could* double check that that offset is within bounds of\nindex->map_size, and I think that is ultimately worth doing at some\npoint. But I think that stopping where you did makes sense, since it\ndoes the minimal thing to fix this bug.\n\nThanks,\nTaylor\n"},{"id":"530635","messageId":"aRVL4iptEeLm/+cs@nand.local","threadId":"64468","inReplyTo":"aRRuzrmbJBW8q4Dd@pks.im","subject":"Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-11-13T03:09:22Z","receivedAt":"2025-11-13T03:09:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 12, 2025 at 12:26:06PM +0100, Patrick Steinhardt wrote:\n> Hm. I'm not a huge fan of not having any error handling at all. It just\n> feels way too fragile for my taste:\n>\n> - As you mention we don't detect overflows, as we would detect them at\n> a later point in time when trying to access index entries at invalid\n> offsets. But if the input is crafted in a way that the overflow ends\n> up with a reasonable index entry we might just as well _not_ detect\n> that an overflow has happened and end up using the wrong index\n> entry.\n>\n> - We don't verify that we even have a number in the first place. We'd\n> simply return \"0\" in that case and not advance the pointer. This is\n> fine though as we verify that the returned size is non-zero, so we'd\n> detect this case.\n>\n> I'd much rather prefer to have an interface similar to `git_parse_int()`\n> and related functions, which are way easier to use compared to the likes\n> of `stroi()`.\n\nThose git_parse_XYZ() functions all end up calling either\ngit_parse_signed() or git_parse_unsigned() under the hood, which bolts\non our k/m/g suffixes, which we probably don't want here when parsing an\non-disk format.\n\nI don't have a strong opinion here, though I tend to agree with\nPatrick's thinking above (with the exception of the suffix thing that I\npointed to earlier).\n\nThanks,\nTaylor\n"},{"id":"530636","messageId":"aRVMggZi7I3vizc9@nand.local","threadId":"64468","inReplyTo":"aRRu1cxpIzd60AoU@pks.im","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-11-13T03:12:02Z","receivedAt":"2025-11-13T03:12:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 12, 2025 at 12:26:13PM +0100, Patrick Steinhardt wrote:\n> On Wed, Nov 12, 2025 at 03:02:15AM -0500, Jeff King wrote:\n> > diff --git a/Makefile b/Makefile\n> > index 7e0f77e298..0f44268405 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n> > endif\n> > ifneq ($(filter address,$(SANITIZERS)),)\n> > NO_REGEX = NeededForASAN\n> > +NO_MMAP = NeededForASAN\n> > SANITIZE_ADDRESS = YesCompiledWithIt\n> > endif\n> > endif\n>\n> Let's also apply this to Meson. Thanks!\n\nNot to derail us too far off topic, but... ;-)\n\nI wonder what (if anything) our policy should be for keeping the\nMakefile and Meson build scripts in sync. On the one hand, I do not want\nthe two of them to drift (too far) apart. But on the other, I am not\nsure that everyone who may be touching the Makefile are necessarily\nfamiliar enough to make the equivalent changes to the Meson build files.\n\nI genuinely don't have a very strong opinion here or even really a clear\nsense of what the right thing to do is. Just something that crossed my\nmind while reading and figured I'd write down in case others had similar\nthoughts.\n\nThanks,\nTaylor\n"},{"id":"530637","messageId":"aRVNyGHJMqR+9WCy@nand.local","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] asan bonanza","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-11-13T03:17:28Z","receivedAt":"2025-11-13T03:17:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 12, 2025 at 02:55:22AM -0500, Jeff King wrote:\n> [1/9]: compat/mmap: mark unused argument in git_munmap()\n> [2/9]: pack-bitmap: handle name-hash lookups in incremental bitmaps\n> [3/9]: Makefile: turn on NO_MMAP when building with ASan\n> [4/9]: cache-tree: avoid strtol() on non-string buffer\n> [5/9]: fsck: assert newline presence in fsck_ident()\n> [6/9]: fsck: avoid strcspn() in fsck_ident()\n> [7/9]: fsck: remove redundant date timestamp check\n> [8/9]: fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated\n> [9/9]: t: enable ASan's strict_string_checks option\n\nNaturally I focused on the first two patches more than the others, but\nthe rest look good to me. I left one minor comment that you might\nconsider if you end up re-rolling, but I don't feel strongly about it.\n\nI like Patrick's suggestion to use an interface similar to\ngit_parse_int() instead of introducing parse_long() as a strtol()\nreplacement. That may be worth a re-roll, especially because there are\ntwo spots that would benefit from that style of interface. But I don't\nfeel strongly about it either way.\n\nLike I mentioned earlier, I mostly glossed over the fsck patches, but\nthey all look reasonable to me.\n\nThanks,\nTaylor\n"},{"id":"530640","messageId":"aRV76Zays4L0u8CP@pks.im","threadId":"64468","inReplyTo":"aRVMggZi7I3vizc9@nand.local","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-13T06:34:17Z","receivedAt":"2025-11-13T06:34:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 12, 2025 at 10:12:02PM -0500, Taylor Blau wrote:\n> On Wed, Nov 12, 2025 at 12:26:13PM +0100, Patrick Steinhardt wrote:\n> > On Wed, Nov 12, 2025 at 03:02:15AM -0500, Jeff King wrote:\n> > > diff --git a/Makefile b/Makefile\n> > > index 7e0f77e298..0f44268405 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n> > > endif\n> > > ifneq ($(filter address,$(SANITIZERS)),)\n> > > NO_REGEX = NeededForASAN\n> > > +NO_MMAP = NeededForASAN\n> > > SANITIZE_ADDRESS = YesCompiledWithIt\n> > > endif\n> > > endif\n> >\n> > Let's also apply this to Meson. Thanks!\n> \n> Not to derail us too far off topic, but... ;-)\n> \n> I wonder what (if anything) our policy should be for keeping the\n> Makefile and Meson build scripts in sync. On the one hand, I do not want\n> the two of them to drift (too far) apart. But on the other, I am not\n> sure that everyone who may be touching the Makefile are necessarily\n> familiar enough to make the equivalent changes to the Meson build files.\n\nThey might not be, true. That's basically the reason why I'm always on\nthe lookout for such changes and try to help them with diffs that they\ncan simply apply.\n\n> I genuinely don't have a very strong opinion here or even really a clear\n> sense of what the right thing to do is. Just something that crossed my\n> mind while reading and figured I'd write down in case others had similar\n\nI don't think there needs to be a policy any stricter than \"Meson\npipelines need to stay green\". So new code files and tests need to be\nadded, but that's easy enough to do even for somebody who isn't familiar\nwith Meson, I think.\n\nOther changes like this one here are not really _that_ important in most\ncases. Git compiles just fine without such a change, and any real-world\nuser is likely to not care. So I think it's fine to handle these on a\nbest effort basis.\n\nThanks!\n\nPatrick\n"},{"id":"530650","messageId":"xmqqfrahq4j8.fsf@gitster.g","threadId":"64468","inReplyTo":"aRRu1cxpIzd60AoU@pks.im","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-13T16:30:03Z","receivedAt":"2025-11-13T16:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Nov 12, 2025 at 03:02:15AM -0500, Jeff King wrote:\n>> diff --git a/Makefile b/Makefile\n>> index 7e0f77e298..0f44268405 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n>> endif\n>> ifneq ($(filter address,$(SANITIZERS)),)\n>> NO_REGEX = NeededForASAN\n>> +NO_MMAP = NeededForASAN\n>> SANITIZE_ADDRESS = YesCompiledWithIt\n>> endif\n>> endif\n>\n> Let's also apply this to Meson. Thanks!\n>\n> Patrick\n\nDo you two want me to squash this into the Makefile patch?\n\n>\n> diff --git a/meson.build b/meson.build\n> index ad4eb2c4fa..668f8769d2 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -1408,12 +1408,18 @@ if host_machine.system() == 'windows'\n> libgit_c_args += '-DUSE_WIN32_MMAP'\n> else\n> checkfuncs += {\n> - 'mmap' : ['mmap.c'],\n> # provided by compat/mingw.c.\n> 'unsetenv' : ['unsetenv.c'],\n> # provided by compat/mingw.c.\n> 'getpagesize' : [],\n> }\n> +\n> + if get_option('b_sanitize').contains('address')\n> + libgit_c_args += '-DNO_MMAP'\n> + libgit_sources += 'compat/mmap.c'\n> + else\n> + checkfuncs += { 'mmap': ['mmap.c'] }\n> + endif\n> endif\n> \n> foreach func, impls : checkfuncs\n"},{"id":"530683","messageId":"aRbToFLhzewwBaSv@pks.im","threadId":"64468","inReplyTo":"xmqqfrahq4j8.fsf@gitster.g","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-14T07:00:48Z","receivedAt":"2025-11-14T07:00:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 13, 2025 at 08:30:03AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Wed, Nov 12, 2025 at 03:02:15AM -0500, Jeff King wrote:\n> >> diff --git a/Makefile b/Makefile\n> >> index 7e0f77e298..0f44268405 100644\n> >> --- a/Makefile\n> >> +++ b/Makefile\n> >> @@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n> >> endif\n> >> ifneq ($(filter address,$(SANITIZERS)),)\n> >> NO_REGEX = NeededForASAN\n> >> +NO_MMAP = NeededForASAN\n> >> SANITIZE_ADDRESS = YesCompiledWithIt\n> >> endif\n> >> endif\n> >\n> > Let's also apply this to Meson. Thanks!\n> >\n> > Patrick\n> \n> Do you two want me to squash this into the Makefile patch?\n\nI feel like there's going to be a revised version of this series anyway,\nso that's probably not necessary. Thanks!\n\nPatrick\n"},{"id":"530724","messageId":"20251115021248.GB3499607@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRRux2uBfORc214r@pks.im","subject":"Re: [PATCH 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-15T02:12:48Z","receivedAt":"2025-11-15T02:12:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 12:25:59PM +0100, Patrick Steinhardt wrote:\n\n> On Wed, Nov 12, 2025 at 03:10:40AM -0500, Jeff King wrote:\n> > In fsck_ident(), we parse the timestamp with parse_timestamp(), which is\n> > really an alias for strtoumax(). But since our buffer may not be\n> > NUL-terminated, this can trigger a complaint from ASan's\n> > strict_string_checks mode. This is a false positive, since we know that\n> > the buffer contains a trailing newline (which we checked earlier in the\n> > function), and that strtoumax() would stop there.\n> > \n> > But it is worth working around ASan's complaint. One is because that\n> > will let us turn on strict_string_checks by default, which has helped\n> > catch other real problems. And two is that the safety of the current\n> > code is very hard to reason about (it subtly depends on distant code\n> > which could change).\n> > \n> > One option here is to just parse the number left-to-right ourselves. But\n> > we care about the size of a timestamp_t and detecting overflow, since\n> > that's part of the point of these checks. And doing that correctly is\n> > tricky. So we'll instead just pull the digits into a separate,\n> > NUL-terminated buffer, and use that to call parse_timestamp().\n> \n> So this is another site that would benefit from having something like\n> `git_parse_int()` with an extra parameter indicating the number of\n> bytes available for parsing (and a way to disable unit factors).\n\nYes, but also no.\n\nYes, in the sense that if we had a robust global function to parse an\ninteger from a buf we could use it here, as well as in cache-tree.\n\nBut there are lots of no's:\n\n - We could not have one such function, because the implementation\n would differ based on signedness and size of the integer type. And\n cache-tree is a signed long, whereas this is a uintmax_t. We can\n factor out some of the work with a helper that takes a max\n parameter, but you can see we duplicate a bunch of code between\n git_parse_signed() and git_parse_unsigned().\n\n - The interface for git_parse_int() isn't quite a match. It wants to\n parse every byte in the provided string and complains if there is\n any extra cruft, rather than aiding in progressive parsing of a\n buffer. So if you have a string \"10 20\\0\", it will not just parse\n \"10\" and then tell you how far it parsed; it will barf on the space.\n If we added a new parameter for \"this is how many bytes we have\",\n and you fed it the buffer \"10 20\" and the length 5, it would have\n the same problem.\n\n So there's a fundamental mismatch between \"parse this as an integer\n and complain if there is anything else\" versus \"parse an integer,\n advance the pointer, and we'll keep going\".\n\n - The implementation for git_parse_int() isn't a match either. It is\n just asking strtoimax() to do all of the work, which is the very\n thing we need to avoid.\n\nSo I think one _could_ write a strtoimax() replacement that handled\neverything we wanted, and then you could probably build\ngit_parse_signed() etc around that. But it would be a lot more work than\nwhat's there (like checking overflow progressively as we multiply and\nadd), and there are some decisions to be made (like handling leading\nwhitespace or how +/- work on unsigned integers). I'd probably err more\non the side of simplicity and strictness than strtol() does, but that\nalso means that plugging in the new function would change user-visible\nbehavior. Sometimes in a good or OK way (stricter parsing), but maybe\nsometimes in a bad or confusing one (rejecting inputs that used to\nwork).\n\n-Peff\n"},{"id":"530725","messageId":"20251115021359.GC3499607@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRbToFLhzewwBaSv@pks.im","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-15T02:13:59Z","receivedAt":"2025-11-15T02:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 14, 2025 at 08:00:48AM +0100, Patrick Steinhardt wrote:\n\n> > Do you two want me to squash this into the Makefile patch?\n> \n> I feel like there's going to be a revised version of this series anyway,\n> so that's probably not necessary. Thanks!\n\nYeah, I'll re-roll (but probably not tonight) and will squash it in.\n\n-Peff\n"},{"id":"530874","messageId":"20251118083838.GA4164207@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRRuzrmbJBW8q4Dd@pks.im","subject":"Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T08:38:38Z","receivedAt":"2025-11-18T08:38:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 12:26:06PM +0100, Patrick Steinhardt wrote:\n\n> > +static long parse_long(const char **ptr, unsigned long *len_p)\n> > +{\n> > +\tconst char *s = *ptr;\n> > +\tunsigned long len = *len_p;\n> > +\tlong ret = 0;\n> > +\tint sign = 1;\n> > +\n> > +\twhile (len && *s == '-') {\n> > +\t\tsign *= -1;\n> > +\t\ts++;\n> > +\t\tlen--;\n> > +\t}\n> > +\n> > +\twhile (len) {\n> > +\t\tif (!isdigit(*s))\n> > +\t\t\tbreak;\n> > +\t\tret *= 10;\n> > +\t\tret += *s - '0';\n> > +\t\ts++;\n> > +\t\tlen--;\n> > +\t}\n> > +\t*ptr = s;\n> > +\t*len_p = len;\n> > +\treturn sign * ret;\n> > +}\n> \n> Hm. I'm not a huge fan of not having any error handling at all. It just\n> feels way too fragile for my taste:\n> \n> - As you mention we don't detect overflows, as we would detect them at\n> a later point in time when trying to access index entries at invalid\n> offsets. But if the input is crafted in a way that the overflow ends\n> up with a reasonable index entry we might just as well _not_ detect\n> that an overflow has happened and end up using the wrong index\n> entry.\n\nYes, but this is true of the original code as well. It does not bother\nto check if strtol() saw overflow (and in fact, it is using \"int\" and\nnot \"long\", so it would need to do its own overflow check on top).\n\nBut see below.\n\n> - We don't verify that we even have a number in the first place. We'd\n> simply return \"0\" in that case and not advance the pointer. This is\n> fine though as we verify that the returned size is non-zero, so we'd\n> detect this case.\n\nI think \"0\" is accepted at least for it->entry_count. The original did\ncheck that strtol() advanced \"ep\", and I dropped that. As with the\noverflow, it's not a memory safety issue, but it changes how we'd react\nto garbage input.\n\nFor v2 I've tweaked the interface to return an error code from the\nhelper, and it will complain if there's no input at all. I didn't add in\noverflow checks, as they weren't there originally and are a little\ntricky to get right (and I wanted to focus on the memory safety issue).\nBut they could be easily added to the helper on top of my patches.\n\n-Peff\n"},{"id":"530875","messageId":"20251118084030.GB4164207@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRVL4iptEeLm/+cs@nand.local","subject":"Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T08:40:30Z","receivedAt":"2025-11-18T08:40:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 10:09:22PM -0500, Taylor Blau wrote:\n\n> On Wed, Nov 12, 2025 at 12:26:06PM +0100, Patrick Steinhardt wrote:\n> > Hm. I'm not a huge fan of not having any error handling at all. It just\n> > feels way too fragile for my taste:\n> >\n> > - As you mention we don't detect overflows, as we would detect them at\n> > a later point in time when trying to access index entries at invalid\n> > offsets. But if the input is crafted in a way that the overflow ends\n> > up with a reasonable index entry we might just as well _not_ detect\n> > that an overflow has happened and end up using the wrong index\n> > entry.\n> >\n> > - We don't verify that we even have a number in the first place. We'd\n> > simply return \"0\" in that case and not advance the pointer. This is\n> > fine though as we verify that the returned size is non-zero, so we'd\n> > detect this case.\n> >\n> > I'd much rather prefer to have an interface similar to `git_parse_int()`\n> > and related functions, which are way easier to use compared to the likes\n> > of `stroi()`.\n> \n> Those git_parse_XYZ() functions all end up calling either\n> git_parse_signed() or git_parse_unsigned() under the hood, which bolts\n> on our k/m/g suffixes, which we probably don't want here when parsing an\n> on-disk format.\n\nIt's much worse than that. They are just wrappers around strtoimax(),\netc, themselves. So we cannot use them for a non-string buffer, and have\nto start from scratch (see my other reply).\n\n-Peff\n"},{"id":"530876","messageId":"20251118084935.GC4164207@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRVMggZi7I3vizc9@nand.local","subject":"Re: [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T08:49:35Z","receivedAt":"2025-11-18T08:49:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 10:12:02PM -0500, Taylor Blau wrote:\n\n> I wonder what (if anything) our policy should be for keeping the\n> Makefile and Meson build scripts in sync. On the one hand, I do not want\n> the two of them to drift (too far) apart. But on the other, I am not\n> sure that everyone who may be touching the Makefile are necessarily\n> familiar enough to make the equivalent changes to the Meson build files.\n> \n> I genuinely don't have a very strong opinion here or even really a clear\n> sense of what the right thing to do is. Just something that crossed my\n> mind while reading and figured I'd write down in case others had similar\n> thoughts.\n\nMy personal preference is for people who care about meson to just post\ntheir own meson patches adding the same features. Either as a separate\nseries (collecting several such features as appropriate), or as a\ncomplete patch that the maintainer can pick up on top of the series in\nquestion.\n\nPosting something squashable (as Patrick did here) is almost as good\n(and certainly better than tasking random folks with figuring out how\nmeson works). But I'm hesitant for reviews of random topics to include\n\"you should re-roll with my proposed meson changes\". It's extra work for\ncontributors who don't care about meson, and it risks de-railing the\ntopic if the changes are non-trivial.\n\nI'll admit I'm possibly biased and being selfish there, because I do not\ncare about meson myself and have mostly found its addition to the\nproject to be a hassle.\n\n-Peff\n"},{"id":"530877","messageId":"20251118085949.GD4164207@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aRVIh9R8Pnuk+yS0@nand.local","subject":"Re: [PATCH 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T08:59:49Z","receivedAt":"2025-11-18T08:59:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 09:55:03PM -0500, Taylor Blau wrote:\n\n> On Wed, Nov 12, 2025 at 03:01:51AM -0500, Jeff King wrote:\n> > As always with the midx and bitmap code, I am left unsure of which\n> > ordering it is correct to use (pseudo-pack order, or lexical oid order,\n> > or how each splits across incremental files). I _think_ this is right\n> > because it's matching the ordering that is already used for a single\n> > midx. But clearly this area is under-tested, since even when we did not\n> > go off the end of the array we were probably passing back junk\n> > name-hashes (either from the .bitmap file's trailing checksum, or\n> > zero-padding at the end of the mapped page).\n> \n> Yeah, this is the right order. \"index_pos\" is a good hint that this is\n> in lexical order. bitmap_writer_finish() has some oid_pos() lookups that\n> use index directly without sorting, so bitmap_writer_finish() expects\n> this array in lexical order.\n\nOK, that matches my analysis. I guess I was just a little surprised that\nthe name hash is in lexical index order, and not pack order. But it\ndefinitely is according to the documentation and the implementation. I\nguess in the end it doesn't really matter that much either way, as you\ntend to reverse the pack/bit position into a lexical index position\nanyway to get the oid. So there is no situation where you don't have\nboth anyway.\n\n> Commit c528e17966 (pack-bitmap: write multi-pack bitmaps, 2021-08-31)\n> has a comment in (what is now) midx-write.c explaining this assumption\n> in bitmap_writer_finish(), but it should probably be documented\n> explicitly in pack-bitmap.h.\n\nMaybe, but I think I may just have been overly paranoid that I got it\nwrong.\n\n> > +static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n> > +{\n> > +\tif (bitmap_is_midx(index)) {\n> > +\t\twhile (index && pos < index->midx->num_objects_in_base)\n> > +\t\t\tindex = index->base;\n> \n> Looks good. It's too bad that we have to reimplement something very\n> similar to midx_for_object(), but I agree with what you wrote in the\n> patch message and this faithfully captures that. It might be worth doing\n> something like:\n> \n> while (index && pos < index->midx->num_objects_in_base) {\n> ASSERT(bitmap_is_midx(index));\n> index = index->base;\n> }\n> \n> , which should never trigger, but is a good sanity check. Definitely not\n> worth re-rolling IMHO.\n\nYeah, I wondered the same thing while writing it. It would be a pretty\nhorrid bug to have mixed entries in the linked list. But that is also\nwhat assertions are there for. ;) I added it for v2.\n\n> > +\t\tif (!index)\n> > +\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n> > +\n> > +\t\tpos -= index->midx->num_objects_in_base;\n> > +\t\tif (pos >= index->midx->num_objects)\n> > +\t\t\tBUG(\"out-of-bounds midx bitmap object at %\"PRIu32, pos);\n> \n> midx_for_object() spells this portion slightly differently, but what you\n> have here is still good.\n\nYes, there it's a die(). But elsewhere, like in pack_pos_to_midx() and\nits reverse, the same situation is a BUG(). It's not clear to me we get\na bit or index position that is out of bounds here (is it truly a bug or\nprogramming error, or might we get it from a corrupt on-disk file). So I\nthink it's mostly academic, at least until somebody can generate a real\ncorrupted case.\n\n> > +\tif (!index->hashes)\n> > +\t\treturn 0;\n> > +\n> > +\treturn get_be32(index->hashes + pos);\n> \n> We *could* double check that that offset is within bounds of\n> index->map_size, and I think that is ultimately worth doing at some\n> point. But I think that stopping where you did makes sense, since it\n> does the minimal thing to fix this bug.\n\nI don't think we need to. When we open the bitmap, we check that its\nhash-cache size matches our expectation based on the number of objects\ncovered by the bitmap (using bitmap_num_objects(), so either from the\npack's count or the midx slice's count).\n\n-Peff\n"},{"id":"530879","messageId":"20251118091127.GA4175601@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251112075522.GA978866@coredump.intra.peff.net","subject":"[PATCH v2 0/9] asan bonanza","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:11:27Z","receivedAt":"2025-11-18T09:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 02:55:22AM -0500, Jeff King wrote:\n\n> This series fixes a handful of issues that ASan finds in our test suite\n> if we tweak a few options to let it look deeper.\n\nHere's a v2 based on feedback:\n\n - added the extra assertion in the midx code\n\n - meson changes are squashed into patch 3\n\n - The cache-tree integer parsing is more robust around total garbage\n inputs (with no digits at all). I agree with the reviewers that it\n would be nice to have a robust, reusable integer parsing function.\n But I think it's non-trivial to do (and I left more comments in the\n thread). I'd like to stick here to just fixing the memory issues\n without making anything worse (which I think this version does).\n\n Note that since the new helper takes an out-parameter, we have to\n match the type more strictly to what the callers have. So it is now\n parse_int(), and not parse_long().\n\nRange diff is below.\n\n [1/9]: compat/mmap: mark unused argument in git_munmap()\n [2/9]: pack-bitmap: handle name-hash lookups in incremental bitmaps\n [3/9]: Makefile: turn on NO_MMAP when building with ASan\n [4/9]: cache-tree: avoid strtol() on non-string buffer\n [5/9]: fsck: assert newline presence in fsck_ident()\n [6/9]: fsck: avoid strcspn() in fsck_ident()\n [7/9]: fsck: remove redundant date timestamp check\n [8/9]: fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated\n [9/9]: t: enable ASan's strict_string_checks option\n\n Makefile | 1 +\n cache-tree.c | 50 ++++++++++++++++++++++++++----------\n compat/mmap.c | 2 +-\n fsck.c | 71 ++++++++++++++++++++++++++++++++++++---------------\n meson.build | 8 +++++-\n pack-bitmap.c | 29 ++++++++++++++++++---\n t/test-lib.sh | 1 +\n 7 files changed, 122 insertions(+), 40 deletions(-)\n\n 1: e24015d41b = 1: 3ce5bd39b5 compat/mmap: mark unused argument in git_munmap()\n 2: e217fb0e3b ! 2: 9908283c33 pack-bitmap: handle name-hash lookups in incremental bitmaps\n @@ pack-bitmap.c: static uint32_t bitmap_num_objects(struct bitmap_index *index)\n +static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n +{\n +\tif (bitmap_is_midx(index)) {\n -+\t\twhile (index && pos < index->midx->num_objects_in_base)\n ++\t\twhile (index && pos < index->midx->num_objects_in_base) {\n ++\t\t\tASSERT(bitmap_is_midx(index));\n +\t\t\tindex = index->base;\n ++\t\t}\n +\n +\t\tif (!index)\n +\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n 3: 8c85dad3c5 < -: ---------- Makefile: turn on NO_MMAP when building with ASan\n -: ---------- > 3: fe3421f6ec Makefile: turn on NO_MMAP when building with ASan\n 4: 38d42984da ! 4: 5e228f2c90 cache-tree: avoid strtol() on non-string buffer\n @@ Commit message\n further. You'd mostly get stopped by seeing non-digits in the oid\n field (and if it is likewise truncated, there will still be 20 or\n more bytes of the index checksum). So it's possible, though\n - unlikely, to see read off the end of the mmap'd buffer. Of course a\n + unlikely, to read off the end of the mmap'd buffer. Of course a\n malicious index file can fake the oid and the index checksum to all\n (ASCII) 0's.\n \n @@ cache-tree.c: void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n \ttrace2_region_leave(\"cache_tree\", \"write\", the_repository);\n }\n \n -+static long parse_long(const char **ptr, unsigned long *len_p)\n ++static int parse_int(const char **ptr, unsigned long *len_p, int *out)\n +{\n +\tconst char *s = *ptr;\n +\tunsigned long len = *len_p;\n -+\tlong ret = 0;\n ++\tint ret = 0;\n +\tint sign = 1;\n +\n +\twhile (len && *s == '-') {\n @@ cache-tree.c: void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n +\t\ts++;\n +\t\tlen--;\n +\t}\n ++\n ++\tif (s == *ptr)\n ++\t\treturn -1;\n ++\n +\t*ptr = s;\n +\t*len_p = len;\n -+\treturn sign * ret;\n ++\t*out = sign * ret;\n ++\treturn 0;\n +}\n +\n static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)\n @@ cache-tree.c: static struct cache_tree *read_one(const char **buffer, unsigned l\n -\tcp = buf;\n -\tit->entry_count = strtol(cp, &ep, 10);\n -\tif (cp == ep)\n -+\tit->entry_count = parse_long(&buf, &size);\n -+\tif (!size || *buf != ' ')\n ++\tif (parse_int(&buf, &size, &it->entry_count) < 0)\n \t\tgoto free_return;\n -\tcp = ep;\n -\tsubtree_nr = strtol(cp, &ep, 10);\n -\tif (cp == ep)\n --\t\tgoto free_return;\n ++\tif (!size || *buf != ' ')\n + \t\tgoto free_return;\n -\twhile (size && *buf && *buf != '\\n') {\n -\t\tsize--;\n -\t\tbuf++;\n -\t}\n -\tif (!size)\n +\tbuf++; size--;\n -+\tsubtree_nr = parse_long(&buf, &size);\n ++\tif (parse_int(&buf, &size, &subtree_nr) < 0)\n ++\t\tgoto free_return;\n +\tif (!size || *buf != '\\n')\n \t\tgoto free_return;\n \tbuf++; size--;\n 5: 73e921a34e = 5: 1d6814233c fsck: assert newline presence in fsck_ident()\n 6: 95e8961df9 = 6: 8cf8152449 fsck: avoid strcspn() in fsck_ident()\n 7: 34baa85dae = 7: 563c3006e4 fsck: remove redundant date timestamp check\n 8: f5ff2dc8ef = 8: 6f88309d76 fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated\n 9: 1b5c0e7ce7 = 9: ad1a1f6a82 t: enable ASan's strict_string_checks option\n"},{"id":"530880","messageId":"20251118091156.GA529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 1/9] compat/mmap: mark unused argument in git_munmap()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:11:56Z","receivedAt":"2025-11-18T09:11:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our mmap compat code emulates mapping by using malloc/free. Our\ngit_munmap() must take a \"length\" parameter to match the interface of\nmunmap(), but we don't use it (it is up to the allocator to know how big\nthe block is in free()).\n\nLet's mark it as UNUSED to avoid complaints from -Wunused-parameter.\nOtherwise you cannot build with \"make DEVELOPER=1 NO_MMAP=1\".\n\nSigned-off-by: Jeff King \n---\n compat/mmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex 2fe1c7732e..1a118711f7 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -38,7 +38,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \treturn start;\n }\n \n-int git_munmap(void *start, size_t length)\n+int git_munmap(void *start, size_t length UNUSED)\n {\n \tfree(start);\n \treturn 0;\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530881","messageId":"20251118091206.GB529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:06Z","receivedAt":"2025-11-18T09:12:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a bitmap has a name-hash cache, it is an array of 32-bit integers,\none per entry in the bitmap, which we've mmap'd from the .bitmap file.\nWe access it directly like this:\n\n if (bitmap_git->hashes)\n hash = get_be32(bitmap_git->hashes + index_pos);\n\nThat works for both regular pack bitmaps and for non-incremental midx\nbitmaps. There is one bitmap_index with one \"hashes\" array, and\nindex_pos is within its bounds (we do the bounds-checking when we load\nthe bitmap).\n\nBut for an incremental midx bitmap, we have a linked list of\nbitmap_index structs, and each one has only its own small slice of the\nname-hash array. If index_pos refers to an object that is not in the\nfirst bitmap_git of the chain, then we'll access memory outside of the\nbounds of its \"hashes\" array, and often outside of the mmap.\n\nInstead, we should walk through the list until we find the bitmap_index\nwhich serves our index_pos, and use its hash (after adjusting index_pos\nto make it relative to the slice we found). This is exactly what we do\nelsewhere for incremental midx lookups (like the pack_pos_to_midx() call\na few lines above). But we can't use existing helpers like\nmidx_for_object() here, because we're walking through the chain of\nbitmap_index structs (each of which refers to a midx), not the chain of\nincremental multi_pack_index structs themselves.\n\nThe problem is triggered in the test suite, but we don't get a segfault\nbecause the out-of-bounds index is too small. The OS typically rounds\nour mmap up to the nearest page size, so we just end up accessing some\nextra zero'd memory. Nor do we catch it with ASan, since it doesn't seem\nto instrument mmaps at all. But if we build with NO_MMAP, then our maps\nare replaced with heap allocations, which ASan does check. And so:\n\n make NO_MMAP=1 SANITIZE=address\n cd t\n ./t5334-incremental-multi-pack-index.sh\n\ndoes show the problem (and this patch makes it go away).\n\nSigned-off-by: Jeff King \n---\n pack-bitmap.c | 29 +++++++++++++++++++++++++----\n 1 file changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 291e1a9cf4..8ca79725b1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -213,6 +213,28 @@ static uint32_t bitmap_num_objects(struct bitmap_index *index)\n \treturn index->pack->num_objects;\n }\n \n+static uint32_t bitmap_name_hash(struct bitmap_index *index, uint32_t pos)\n+{\n+\tif (bitmap_is_midx(index)) {\n+\t\twhile (index && pos < index->midx->num_objects_in_base) {\n+\t\t\tASSERT(bitmap_is_midx(index));\n+\t\t\tindex = index->base;\n+\t\t}\n+\n+\t\tif (!index)\n+\t\t\tBUG(\"NULL base bitmap for object position: %\"PRIu32, pos);\n+\n+\t\tpos -= index->midx->num_objects_in_base;\n+\t\tif (pos >= index->midx->num_objects)\n+\t\t\tBUG(\"out-of-bounds midx bitmap object at %\"PRIu32, pos);\n+\t}\n+\n+\tif (!index->hashes)\n+\t\treturn 0;\n+\n+\treturn get_be32(index->hashes + pos);\n+}\n+\n static struct repository *bitmap_repo(struct bitmap_index *bitmap_git)\n {\n \tif (bitmap_is_midx(bitmap_git))\n@@ -1724,8 +1746,7 @@ static void show_objects_for_type(\n \t\t\t\tpack = bitmap_git->pack;\n \t\t\t}\n \n-\t\t\tif (bitmap_git->hashes)\n-\t\t\t\thash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t\thash = bitmap_name_hash(bitmap_git, index_pos);\n \n \t\t\tshow_reach(&oid, object_type, 0, hash, pack, ofs, payload);\n \t\t}\n@@ -3124,8 +3145,8 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \n \t\tif (oe) {\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\n-\t\t\tif (bitmap_git->hashes && !oe->hash)\n-\t\t\t\toe->hash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t\tif (!oe->hash)\n+\t\t\t\toe->hash = bitmap_name_hash(bitmap_git, index_pos);\n \t\t}\n \t}\n \n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530882","messageId":"20251118091213.GC529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 3/9] Makefile: turn on NO_MMAP when building with ASan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:13Z","receivedAt":"2025-11-18T09:12:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git often uses mmap() to access on-disk files. This leaves a blind spot\nin our SANITIZE=address builds, since ASan does not seem to handle mmap\nat all. Nor does the OS notice most out-of-bounds access, since it tends\nto round up to the nearest page size (so depending on how big the map\nis, you might have to overrun it by up to 4095 bytes to trigger a\nsegfault).\n\nThe previous commit demonstrates a memory bug that we missed. We could\nhave made a new test where the out-of-bounds access was much larger, or\nwhere the mapped file ended closer to a page boundary. But the point of\nrunning the test suite with sanitizers is to catch these problems\nwithout having to construct specific tests.\n\nLet's enable NO_MMAP for our ASan builds by default, which should give\nus better coverage. This does increase the memory usage of Git, since\nwe're copying from the filesystem into heap. But the repositories in the\ntest suite tend to be small, so the overhead isn't really noticeable\n(and ASan already has quite a performance penalty).\n\nThere are a few other known bugs that this patch will help flush out.\nHowever, they aren't directly triggered in the test suite (yet). So\nit's safe to turn this on now without breaking the test suite, which\nwill help us add new tests to demonstrate those other bugs as we fix\nthem.\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n meson.build | 8 +++++++-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 7e0f77e298..0f44268405 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1587,6 +1587,7 @@ SANITIZE_LEAK = YesCompiledWithIt\n endif\n ifneq ($(filter address,$(SANITIZERS)),)\n NO_REGEX = NeededForASAN\n+NO_MMAP = NeededForASAN\n SANITIZE_ADDRESS = YesCompiledWithIt\n endif\n endif\ndiff --git a/meson.build b/meson.build\nindex 1f95a06edb..f1b3615659 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1411,12 +1411,18 @@ if host_machine.system() == 'windows'\n libgit_c_args += '-DUSE_WIN32_MMAP'\n else\n checkfuncs += {\n- 'mmap' : ['mmap.c'],\n # provided by compat/mingw.c.\n 'unsetenv' : ['unsetenv.c'],\n # provided by compat/mingw.c.\n 'getpagesize' : [],\n }\n+\n+ if get_option('b_sanitize').contains('address')\n+ libgit_c_args += '-DNO_MMAP'\n+ libgit_sources += 'compat/mmap.c'\n+ else\n+ checkfuncs += { 'mmap': ['mmap.c'] }\n+ endif\n endif\n \n foreach func, impls : checkfuncs\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530883","messageId":"20251118091218.GD529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:18Z","receivedAt":"2025-11-18T09:12:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A cache-tree extension entry in the index looks like this:\n\n NUL SPACE NEWLINE \n\nwhere the \"_nr\" items are human-readable base-10 ASCII. We parse them\nwith strtol(), even though we do not have a NUL-terminated string (we'd\ngenerally have an mmap() of the on-disk index file). For a well-formed\nentry, this is not a problem; strtol() will stop when it sees the\nnewline. But there are two problems:\n\n 1. A corrupted entry could omit the newline, causing us to read\n further. You'd mostly get stopped by seeing non-digits in the oid\n field (and if it is likewise truncated, there will still be 20 or\n more bytes of the index checksum). So it's possible, though\n unlikely, to read off the end of the mmap'd buffer. Of course a\n malicious index file can fake the oid and the index checksum to all\n (ASCII) 0's.\n\n This is further complicated by the fact that mmap'd buffers tend to\n be zero-padded up to the page boundary. So to run off the end, the\n index size also has to be a multiple of the page size. This is also\n unlikely, though you can construct a malicious index file that\n matches this.\n\n The security implications aren't too interesting. The index file is\n a local file anyway (so you can't attack somebody by cloning, but\n only if you convince them to operate in a .git directory you made,\n at which point attacking .git/config is much easier). And it's just\n a read overflow via strtol(), which is unlikely to buy you much\n beyond a crash.\n\n 2. ASan has a strict_string_checks option, which tells it to make sure\n that options to string functions (like strtol) have some eventual\n NUL, without regard to what the function would actually do (like\n stopping at a newline here). This option sometimes has false\n positives, but it can point to sketchy areas (like this one) where\n the input we use doesn't exhibit a problem, but different input\n _could_ cause us to misbehave.\n\nLet's fix it by just parsing the values ourselves with a helper function\nthat is careful not to go past the end of the buffer. There are a few\nbehavior changes here that should not matter:\n\n - We do not consider overflow, as strtol() would. But nor did the\n original code. However, we don't trust the value we get from the\n on-disk file, and if it says to read 2^30 entries, we would notice\n that we do not have that many and bail before reading off the end of\n the buffer.\n\n - Our helper does not skip past extra leading whitespace as strtol()\n would, but according to gitformat-index(5) there should not be any.\n\n - The original quit parsing at a newline or a NUL byte, but now we\n insist on a newline (which is what the documentation says, and what\n Git has always produced).\n\nSince we are providing our own helper function, we can tweak the\ninterface a bit to make our lives easier. The original code does not use\nstrtol's \"end\" pointer to find the end of the parsed data, but rather\nuses a separate loop to advance our \"buf\" pointer to the trailing\nnewline. We can instead provide a helper that advances \"buf\" as it\nparses, letting us read strictly left-to-right through the buffer.\n\nI didn't add a new test here. It's surprisingly difficult to construct\nan index of exactly the right size due to the way we pad entries. But it\nis easy to trigger the problem in existing tests when using ASan's\nstrict string checking, coupled with a recent change to use NO_MMAP with\nASan builds. So:\n\n make SANITIZE=address\n cd t\n ASAN_OPTIONS=strict_string_checks=1 ./t0090-cache-tree.sh\n\ntriggers it reliably. Technically it is not deterministic because there\nis ~8% chance (it's 1-(255/256)^20, or ^32 for sha256) that the trailing\nchecksum hash has a NUL byte in it. But we compute enough cache-trees in\nthe course of that script that we are very likely to hit the problem in\none of them.\n\nWe can look at making strict_string_checks the default for ASan builds,\nbut there are some other cases we'd want to fix first.\n\nReported-by: correctmost \nSigned-off-by: Jeff King \n---\n cache-tree.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 37 insertions(+), 13 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2aba47060e..2d8947b518 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -548,12 +548,41 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n \ttrace2_region_leave(\"cache_tree\", \"write\", the_repository);\n }\n \n+static int parse_int(const char **ptr, unsigned long *len_p, int *out)\n+{\n+\tconst char *s = *ptr;\n+\tunsigned long len = *len_p;\n+\tint ret = 0;\n+\tint sign = 1;\n+\n+\twhile (len && *s == '-') {\n+\t\tsign *= -1;\n+\t\ts++;\n+\t\tlen--;\n+\t}\n+\n+\twhile (len) {\n+\t\tif (!isdigit(*s))\n+\t\t\tbreak;\n+\t\tret *= 10;\n+\t\tret += *s - '0';\n+\t\ts++;\n+\t\tlen--;\n+\t}\n+\n+\tif (s == *ptr)\n+\t\treturn -1;\n+\n+\t*ptr = s;\n+\t*len_p = len;\n+\t*out = sign * ret;\n+\treturn 0;\n+}\n+\n static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)\n {\n \tconst char *buf = *buffer;\n \tunsigned long size = *size_p;\n-\tconst char *cp;\n-\tchar *ep;\n \tstruct cache_tree *it;\n \tint i, subtree_nr;\n \tconst unsigned rawsz = the_hash_algo->rawsz;\n@@ -569,19 +598,14 @@ static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)\n \tbuf++; size--;\n \tit = cache_tree();\n \n-\tcp = buf;\n-\tit->entry_count = strtol(cp, &ep, 10);\n-\tif (cp == ep)\n+\tif (parse_int(&buf, &size, &it->entry_count) < 0)\n \t\tgoto free_return;\n-\tcp = ep;\n-\tsubtree_nr = strtol(cp, &ep, 10);\n-\tif (cp == ep)\n+\tif (!size || *buf != ' ')\n \t\tgoto free_return;\n-\twhile (size && *buf && *buf != '\\n') {\n-\t\tsize--;\n-\t\tbuf++;\n-\t}\n-\tif (!size)\n+\tbuf++; size--;\n+\tif (parse_int(&buf, &size, &subtree_nr) < 0)\n+\t\tgoto free_return;\n+\tif (!size || *buf != '\\n')\n \t\tgoto free_return;\n \tbuf++; size--;\n \tif (0 <= it->entry_count) {\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530884","messageId":"20251118091220.GE529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 5/9] fsck: assert newline presence in fsck_ident()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:20Z","receivedAt":"2025-11-18T09:12:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fsck code purports to handle buffers that are not NUL-terminated,\nbut fsck_ident() uses some string functions. This works OK in practice,\nas explained in 8e4309038f (fsck: do not assume NUL-termination of\nbuffers, 2023-01-19). Before calling fsck_ident() we'll have called\nverify_headers(), which makes sure we have at least a trailing newline.\nAnd none of our string-like functions will walk past that newline.\n\nHowever, that makes this code at the top of fsck_ident() very confusing:\n\n *ident = strchrnul(*ident, '\\n');\n if (**ident == '\\n')\n (*ident)++;\n\nWe should always see that newline, or our memory safety assumptions have\nbeen violated! Further, using strchrnul() is weird, since the whole\npoint is that if the newline is not there, we don't necessarily have a\nNUL at all, and might read off the end of the buffer.\n\nSo let's have callers pass in the boundary of our buffer, which lets us\nsafely find the newline with memchr(). And if it is not there, this is a\nBUG(), because it means our caller did not validate the input with\nverify_headers() as it was supposed to (and we are better off bailing\nrather than having memory-safety problems).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 341e100d24..8991f04943 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -860,16 +860,18 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n-static int fsck_ident(const char **ident,\n+static int fsck_ident(const char **ident, const char *ident_end,\n \t\t const struct object_id *oid, enum object_type type,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n+\tconst char *nl;\n \tchar *end;\n \n-\t*ident = strchrnul(*ident, '\\n');\n-\tif (**ident == '\\n')\n-\t\t(*ident)++;\n+\tnl = memchr(p, '\\n', ident_end - p);\n+\tif (!nl)\n+\t\tBUG(\"verify_headers() should have made sure we have a newline\");\n+\t*ident = nl + 1;\n \n \tif (*p == '<')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n@@ -958,7 +960,7 @@ static int fsck_commit(const struct object_id *oid,\n \tauthor_count = 0;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\terr = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options);\n \t\tif (err)\n \t\t\treturn err;\n \t}\n@@ -970,7 +972,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\treturn err;\n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\terr = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n@@ -1065,7 +1067,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t\t\tgoto done;\n \t}\n \telse\n-\t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n+\t\tret = fsck_ident(&buffer, buffer_end, oid, OBJ_TAG, options);\n \n \tif (buffer < buffer_end && (skip_prefix(buffer, \"gpgsig \", &buffer) || skip_prefix(buffer, \"gpgsig-sha256 \", &buffer))) {\n \t\teol = memchr(buffer, '\\n', buffer_end - buffer);\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530885","messageId":"20251118091223.GF529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 6/9] fsck: avoid strcspn() in fsck_ident()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:23Z","receivedAt":"2025-11-18T09:12:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may be operating on a buffer that is not NUL-terminated, but we use\nstrcspn() to parse it. This is OK in practice, as discussed in\n8e4309038f (fsck: do not assume NUL-termination of buffers, 2023-01-19),\nbecause we know there is at least a trailing newline in our buffer, and\nwe always pass \"\\n\" to strcspn(). So we know it will stop before running\noff the end of the buffer.\n\nBut this is a subtle point to hang our memory safety hat on. And it\nconfuses ASan's strict_string_checks mode, even though it is technically\na false positive (that mode complains that we have no NUL, which is\ntrue, but it does not know that we have verified the presence of the\nnewline already).\n\nLet's instead open-code the loop. As a bonus, this makes the logic more\nobvious (to my mind, anyway). The current code skips forward with\nstrcspn until it hits \"<\", \">\", or \"\\n\". But then it must check which it\nsaw to decide if that was what we expected or not, duplicating some\nlogic between what's in the strcspn() and what's in the domain logic.\nInstead, we can just check each character as we loop and act on it\nimmediately.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 32 ++++++++++++++++++++++----------\n 1 file changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 8991f04943..2ee72d573d 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -875,18 +875,30 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \n \tif (*p == '<')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n-\tp += strcspn(p, \"<>\\n\");\n-\tif (*p == '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n-\tif (*p != '<')\n-\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\tfor (;;) {\n+\t\tif (p >= ident_end || *p == '\\n')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\t\tif (*p == '>')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n+\t\tif (*p == '<')\n+\t\t\tbreak; /* end of name, beginning of email */\n+\n+\t\t/* otherwise, skip past arbitrary name char */\n+\t\tp++;\n+\t}\n \tif (p[-1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n-\tp++;\n-\tp += strcspn(p, \"<>\\n\");\n-\tif (*p != '>')\n-\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n-\tp++;\n+\tp++; /* skip past '<' we found */\n+\tfor (;;) {\n+\t\tif (p >= ident_end || *p == '<' || *p == '\\n')\n+\t\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n+\t\tif (*p == '>')\n+\t\t\tbreak; /* end of email */\n+\n+\t\t/* otherwise, skip past arbitrary email char */\n+\t\tp++;\n+\t}\n+\tp++; /* skip past '>' we found */\n \tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530886","messageId":"20251118091225.GG529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 7/9] fsck: remove redundant date timestamp check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:25Z","receivedAt":"2025-11-18T09:12:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After calling \"parse_timestamp(p, &end, 10)\", we complain if \"p == end\",\nwhich would imply that we did not see any digits at all. But we know\nthis cannot be the case, since we would have bailed already if we did\nnot see any digits, courtesy of extra checks added by 8e4309038f (fsck:\ndo not assume NUL-termination of buffers, 2023-01-19). Since then,\nchecking \"p == end\" is redundant and we can drop it.\n\nThis will make our lives a little easier as we refactor further.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2ee72d573d..266c965cec 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -920,7 +920,7 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n-\tif ((end == p || *end != ' '))\n+\tif (*end != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n \tp = end + 1;\n \tif ((*p != '+' && *p != '-') ||\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530887","messageId":"20251118091228.GH529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:28Z","receivedAt":"2025-11-18T09:12:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In fsck_ident(), we parse the timestamp with parse_timestamp(), which is\nreally an alias for strtoumax(). But since our buffer may not be\nNUL-terminated, this can trigger a complaint from ASan's\nstrict_string_checks mode. This is a false positive, since we know that\nthe buffer contains a trailing newline (which we checked earlier in the\nfunction), and that strtoumax() would stop there.\n\nBut it is worth working around ASan's complaint. One is because that\nwill let us turn on strict_string_checks by default, which has helped\ncatch other real problems. And two is that the safety of the current\ncode is very hard to reason about (it subtly depends on distant code\nwhich could change).\n\nOne option here is to just parse the number left-to-right ourselves. But\nwe care about the size of a timestamp_t and detecting overflow, since\nthat's part of the point of these checks. And doing that correctly is\ntricky. So we'll instead just pull the digits into a separate,\nNUL-terminated buffer, and use that to call parse_timestamp().\n\nSigned-off-by: Jeff King \n---\n fsck.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 266c965cec..8e8083e7c6 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -860,13 +860,28 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n+static timestamp_t parse_timestamp_from_buf(const char **start, const char *end)\n+{\n+\tconst char *p = *start;\n+\tchar buf[24]; /* big enough for 2^64 */\n+\tsize_t i = 0;\n+\n+\twhile (p < end && isdigit(*p)) {\n+\t\tif (i >= ARRAY_SIZE(buf) - 1)\n+\t\t\treturn TIME_MAX;\n+\t\tbuf[i++] = *p++;\n+\t}\n+\tbuf[i] = '\\0';\n+\t*start = p;\n+\treturn parse_timestamp(buf, NULL, 10);\n+}\n+\n static int fsck_ident(const char **ident, const char *ident_end,\n \t\t const struct object_id *oid, enum object_type type,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n \tconst char *nl;\n-\tchar *end;\n \n \tnl = memchr(p, '\\n', ident_end - p);\n \tif (!nl)\n@@ -918,11 +933,11 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \t\t\t \"invalid author/committer line - bad date\");\n \tif (*p == '0' && p[1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n-\tif (date_overflows(parse_timestamp(p, &end, 10)))\n+\tif (date_overflows(parse_timestamp_from_buf(&p, ident_end)))\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n-\tif (*end != ' ')\n+\tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n-\tp = end + 1;\n+\tp++;\n \tif ((*p != '+' && *p != '-') ||\n \t !isdigit(p[1]) ||\n \t !isdigit(p[2]) ||\n-- \n2.52.0.278.gadc6434dc3\n\n"},{"id":"530888","messageId":"20251118091230.GI529192@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"[PATCH v2 9/9] t: enable ASan's strict_string_checks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:12:30Z","receivedAt":"2025-11-18T09:12:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"ASan has an option to enable strict string checking, where any pointer\npassed to a function that expects a NUL-terminated string will be\nchecked for that NUL termination. This can sometimes produce false\npositives. E.g., it is not wrong to pass a buffer with { '1', '2', '\\n' }\ninto strtoul(). Even though it is not NUL-terminated, it will stop at\nthe newline.\n\nBut in trying it out, it identified two problematic spots in our test\nsuite (which have now been adjusted):\n\n 1. The strtol() parsing in cache-tree.c was a real potential problem,\n which would have been very hard to find otherwise (since it\n required constructing a very specific broken index file).\n\n 2. The use of string functions in fsck_ident() were false positives,\n because we knew that there was always a trailing newline which\n would stop the functions from reading off the end of the buffer.\n But the reasoning behind that is somewhat fragile, and silencing\n those complaints made the code easier to reason about.\n\nSo even though this did not find any earth-shattering bugs, and even had\na few false positives, I'm sufficiently convinced that its complaints\nare more helpful than hurtful. Let's turn it on by default (since the\ntest suite now runs cleanly with it) and see if it ever turns up any\nother instances.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ef0ab7ec2d..0fb76f7d11 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -77,6 +77,7 @@ prepend_var GIT_SAN_OPTIONS : strip_path_prefix=\"$GIT_BUILD_DIR/\"\n # want that one to complain to stderr).\n prepend_var ASAN_OPTIONS : $GIT_SAN_OPTIONS\n prepend_var ASAN_OPTIONS : detect_leaks=0\n+prepend_var ASAN_OPTIONS : strict_string_checks=1\n export ASAN_OPTIONS\n \n prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS\n-- \n2.52.0.278.gadc6434dc3\n"},{"id":"530903","messageId":"ca6d99cc-d05c-49fb-ab3c-d7668077d32b@gmail.com","threadId":"64468","inReplyTo":"20251118091218.GD529192@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-18T14:30:32Z","receivedAt":"2025-11-18T14:30:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 18/11/2025 09:12, Jeff King wrote:\n> Let's fix it by just parsing the values ourselves with a helper function\n> that is careful not to go past the end of the buffer. There are a few\n> behavior changes here that should not matter:\n> \n> - We do not consider overflow, as strtol() would. But nor did the\n> original code. However, we don't trust the value we get from the\n> on-disk file, and if it says to read 2^30 entries, we would notice\n> that we do not have that many and bail before reading off the end of\n> the buffer.\n> \n> - Our helper does not skip past extra leading whitespace as strtol()\n> would, but according to gitformat-index(5) there should not be any.\n> \n> - The original quit parsing at a newline or a NUL byte, but now we\n> insist on a newline (which is what the documentation says, and what\n> Git has always produced).\n\nI think that sounds reasonable, I've left a couple of comments below.\n\n> +static int parse_int(const char **ptr, unsigned long *len_p, int *out)\n> +{\n> +\tconst char *s = *ptr;\n> +\tunsigned long len = *len_p;\n> +\tint ret = 0;\n\nThis is signed which means that any overflow is undefined. While the \nexisting code does not check for overflow I think it is well defined in \nthe presence of overflow. It also means parsing INT_MIN is undefined as \nwe parse the value as unsigned and then multiply by -1 if we saw a \nleading '-'. We shouldn't see any negative values apart from \"-1\" but \ngiven we're changing this code to be more robust in handling malformed \ninput it would be nice if parsing INT_MIN was well defined.\n\n> +\tint sign = 1;\n> +\n> +\twhile (len && *s == '-') {\n> +\t\tsign *= -1;\n> +\t\ts++;\n> +\t\tlen--;\n> +\t}\n\nThis accepts any number of '-' signs but I believe strtol() only accepts \na single sign (the standard says \"optionally preceded by a plus or minus \nsign\") so this is a change in behavior from the existing code. I'm not \nsure we really need to be that accommodating here.\n\n> +\twhile (len) {\n> +\t\tif (!isdigit(*s))\n> +\t\t\tbreak;\n> +\t\tret *= 10;\n> +\t\tret += *s - '0';\n> +\t\ts++;\n> +\t\tlen--;\n> +\t}\n> +\n> +\tif (s == *ptr)\n> +\t\treturn -1;\n\nThis accepts \"-\" as a valid input, as we're tightening up our parsing it \nwould be nice to require a digit after any '-' sign.\n\n > [...]> +\tbuf++; size--;\n> +\tif (parse_int(&buf, &size, &subtree_nr) < 0)\n> +\t\tgoto free_return;\n\nThis isn't a new problem but if subtree_nr is negative we end up trying \nto allocate a huge chunk of memory. If that somehow succeeds we then end \nup calling die(\"cache-tree: internal error\"). The existing code looks \nsafe but it would be nice to die() a bit earlier if subtree_nr is negative.\n\nThanks\n\nPhillip\n > +\tif (!size || *buf != '\\n')\n> \t\tgoto free_return;\n> \tbuf++; size--;\n> \tif (0 <= it->entry_count) {\n\n"},{"id":"531172","messageId":"xmqqy0nxz4bh.fsf@gitster.g","threadId":"64468","inReplyTo":"20251118091127.GA4175601@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/9] asan bonanza","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-23T05:49:22Z","receivedAt":"2025-11-23T05:49:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's a v2 based on feedback:\n>\n> - added the extra assertion in the midx code\n>\n> - meson changes are squashed into patch 3\n>\n> - The cache-tree integer parsing is more robust around total garbage\n> inputs (with no digits at all). I agree with the reviewers that it\n> would be nice to have a robust, reusable integer parsing function.\n> But I think it's non-trivial to do (and I left more comments in the\n> thread). I'd like to stick here to just fixing the memory issues\n> without making anything worse (which I think this version does).\n>\n> Note that since the new helper takes an out-parameter, we have to\n> match the type more strictly to what the callers have. So it is now\n> parse_int(), and not parse_long().\n>\n> Range diff is below.\n>\n> [1/9]: compat/mmap: mark unused argument in git_munmap()\n> [2/9]: pack-bitmap: handle name-hash lookups in incremental bitmaps\n> [3/9]: Makefile: turn on NO_MMAP when building with ASan\n> [4/9]: cache-tree: avoid strtol() on non-string buffer\n> [5/9]: fsck: assert newline presence in fsck_ident()\n> [6/9]: fsck: avoid strcspn() in fsck_ident()\n> [7/9]: fsck: remove redundant date timestamp check\n> [8/9]: fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated\n> [9/9]: t: enable ASan's strict_string_checks option\n\nAside from the comment on strtol() replacement, this iteration did\nnot see any comments. What do we want to do next with this series?\n\n\n"},{"id":"531173","messageId":"xmqqtsylz2xh.fsf@gitster.g","threadId":"64468","inReplyTo":"ca6d99cc-d05c-49fb-ab3c-d7668077d32b@gmail.com","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-23T06:19:22Z","receivedAt":"2025-11-23T06:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> +\twhile (len && *s == '-') {\n>> +\t\tsign *= -1;\n>> +\t\ts++;\n>> +\t\tlen--;\n>> +\t}\n>\n> This accepts any number of '-' signs but I believe strtol() only accepts \n> a single sign (the standard says \"optionally preceded by a plus or minus \n> sign\") so this is a change in behavior from the existing code. I'm not \n> sure we really need to be that accommodating here.\n\nThat is true, but at the same time I do not think we really need to\nmake it more strict with extra code.\n\n>> +\twhile (len) {\n>> +\t\tif (!isdigit(*s))\n>> +\t\t\tbreak;\n>> +\t\tret *= 10;\n>> +\t\tret += *s - '0';\n>> +\t\ts++;\n>> +\t\tlen--;\n>> +\t}\n>> +\n>> +\tif (s == *ptr)\n>> +\t\treturn -1;\n>\n> This accepts \"-\" as a valid input, as we're tightening up our parsing it \n> would be nice to require a digit after any '-' sign.\n\nDitto.\n\n\n\nWe could try to be more careful, but it quickly became messy when I\ntried. Here is an unfinished attempt of mine.\n\n\nstatic int parse_int(const char **ptr, unsigned long *len_p, int *out)\n{\n\tconst char *s = *ptr;\n\tunsigned long len = *len_p;\n\tunsigned val = 0;\n\tbool negate = false;\n\tint saw_digits = 0;\n\n\twhile (len && isspace(*s)) {\n\t\tlen--;\n\t\ts++;\n\t}\n\tif (!len)\n\t\treturn -1;\n\tswitch (*s) {\n\tcase '-':\n\t\tnegate = true;\n\t\t/* fallthru */\n\tcase '+':\n\t\ts++;\n\t\tlen--;\n\t\tbreak;\n\tdefault:\n\t\tbreak;\n\t}\n\n\twhile (len) {\n\t\tunsigned next;\n\t\tif (!isdigit(*s))\n\t\t\tbreak;\n\t\tnext = val * 10 + *s - '0';\n\t\tif (next < val)\n\t\t\treturn -1;\n\t\tval = next;\n\t\ts++;\n\t\tlen--;\n\t\tsaw_digits = 1;\n\t}\n\tif (!saw_digits ||\n\t (!negate && INT_MAX <= val) || \n\t (negate && INT_MAX < val))\n\t\treturn -1;\n\n\t*ptr = s;\n\t*len_p = len;\n\t*out = negate ? (0 - val) : val;\n\treturn 0;\n}\n"},{"id":"531175","messageId":"633f4d92-c258-45a8-9d32-116c94838e68@gmail.com","threadId":"64468","inReplyTo":"xmqqtsylz2xh.fsf@gitster.g","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-23T15:51:57Z","receivedAt":"2025-11-23T15:52:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/11/2025 06:19, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>> +\twhile (len && *s == '-') {\n>>> +\t\tsign *= -1;\n>>> +\t\ts++;\n>>> +\t\tlen--;\n>>> +\t}\n>>\n>> This accepts any number of '-' signs but I believe strtol() only accepts\n>> a single sign (the standard says \"optionally preceded by a plus or minus\n>> sign\") so this is a change in behavior from the existing code. I'm not\n>> sure we really need to be that accommodating here.\n> \n> That is true, but at the same time I do not think we really need to\n> make it more strict with extra code.\n\nAll we need to do to accept a single minus sign is s/while/if/\n\n>>> +\twhile (len) {\n>>> +\t\tif (!isdigit(*s))\n>>> +\t\t\tbreak;\n>>> +\t\tret *= 10;\n>>> +\t\tret += *s - '0';\n>>> +\t\ts++;\n>>> +\t\tlen--;\n>>> +\t}\n>>> +\n>>> +\tif (s == *ptr)\n>>> +\t\treturn -1;\n>>\n>> This accepts \"-\" as a valid input, as we're tightening up our parsing it\n>> would be nice to require a digit after any '-' sign.\n> \n> Ditto.\n\nIf we limit ourselves to accepting a single minus sign then this can become\n\tif (s == *ptr + (sign == -1))\n\nso we need very little in the way of extra code.\n\n> We could try to be more careful, but it quickly became messy when I\n> tried. Here is an unfinished attempt of mine.\n\nA generic helper to replace strtol() that takes a length rather than \nassuming the input is NUL terminated could be useful elsewhere but I'm \nnot sure we need something that complicated here. I do like the fact \nthat overflow does not cause undefined behavior though. Changing ret for \n\"int\" to \"unsigned\" in peff's patch should fix that.\n\nThanks\n\nPhillip\n\n> \n> static int parse_int(const char **ptr, unsigned long *len_p, int *out)\n> {\n> \tconst char *s = *ptr;\n> \tunsigned long len = *len_p;\n> \tunsigned val = 0;\n> \tbool negate = false;\n> \tint saw_digits = 0;\n> \n> \twhile (len && isspace(*s)) {\n> \t\tlen--;\n> \t\ts++;\n> \t}\n> \tif (!len)\n> \t\treturn -1;\n> \tswitch (*s) {\n> \tcase '-':\n> \t\tnegate = true;\n> \t\t/* fallthru */\n> \tcase '+':\n> \t\ts++;\n> \t\tlen--;\n> \t\tbreak;\n> \tdefault:\n> \t\tbreak;\n> \t}\n> \n> \twhile (len) {\n> \t\tunsigned next;\n> \t\tif (!isdigit(*s))\n> \t\t\tbreak;\n> \t\tnext = val * 10 + *s - '0';\n> \t\tif (next < val)\n> \t\t\treturn -1;\n> \t\tval = next;\n> \t\ts++;\n> \t\tlen--;\n> \t\tsaw_digits = 1;\n> \t}\n> \tif (!saw_digits ||\n> \t (!negate && INT_MAX <= val) ||\n> \t (negate && INT_MAX < val))\n> \t\treturn -1;\n> \n> \t*ptr = s;\n> \t*len_p = len;\n> \t*out = negate ? (0 - val) : val;\n> \treturn 0;\n> }\n\n"},{"id":"531176","messageId":"xmqqh5ukzkqt.fsf@gitster.g","threadId":"64468","inReplyTo":"633f4d92-c258-45a8-9d32-116c94838e68@gmail.com","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-23T18:06:50Z","receivedAt":"2025-11-23T18:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> All we need to do to accept a single minus sign is s/while/if/\n> ...\n> If we limit ourselves to accepting a single minus sign then this can become\n> \tif (s == *ptr + (sign == -1))\n>\n> so we need very little in the way of extra code.\n> ...\n> A generic helper to replace strtol() that takes a length rather than \n> assuming the input is NUL terminated could be useful elsewhere but I'm \n> not sure we need something that complicated here. I do like the fact \n> that overflow does not cause undefined behavior though. Changing ret for \n> \"int\" to \"unsigned\" in peff's patch should fix that.\n> \n> Thanks\n\nPerhaps.\n\nBy the way, an interesting tangent is this.\n\nThe only reason why these fields under discussion are stored in\ntextual decimal is pretty much the same as the reason why the object\nheader expresses the byte-length of the payload in textual decimal,\ni.e., to be independent from the platform natural implementation of\n\"int\" type (e.g., endiannness and width), but unlike object files,\nthe index is a local matter (we are prepared for the same directory\naccessed over NFS from two platforms with different endianness, but\nwe do not recommend network access to a repository in the first\nplace). And a lot more importantly, the total number of the index\nentries contained within an index file is capped to 2^32-1 (the\nheader has 32-bit count in the network byte order). The total\nnumber of subdirectories within a directory or the total number of\nentries for a level of directory hierarchy that would form a tree\nobject from a slice of the index cannot exceed that number anyway.\n\nAnd thanks to the design that made cache-tree an optional index\nextension, we can make cache-tree version 2 where the in-core\nrepresentation is exactly the same as the current one, but only uses\ndifferent serialization when writing to and reading from the index\nfile. The new serialization can use 32-bit network byte order\nintegers, or use our own varint.{c,h,rs}, to record these numbers.\n\nA version of Git that knows about that extension could be taught to\nread from the current cache-tree and convert to a new version, but\nbetter yet, it can simply ignore the current cache-tree data in the\nfile, and write the new version when we do need to write the index\nout with a cache-tree. When such a transparent auto conversion\nhappens, one single invocation of write_index_as_tree() would become\nmore expensive than usual (because the last invocation of the\ncurrent Git left cache-tree data in the index and usually the next\ninvocation of Git would take advantage of it when it writes a tree,\nbut a new version of Git that uses the v2 format would behave as if\nthere is no cache-tree data in the index and build the tree from\nscratch. After that happens, the cache-tree data in the new format\nwill be reused and things will continue to work. You could use an\nolder version of Git on such an index file and the same transparent\nauto conversion will take care of the transition.\n"},{"id":"531240","messageId":"20251124223023.GA2051672@coredump.intra.peff.net","threadId":"64468","inReplyTo":"xmqqtsylz2xh.fsf@gitster.g","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-24T22:30:23Z","receivedAt":"2025-11-24T22:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 22, 2025 at 10:19:22PM -0800, Junio C Hamano wrote:\n\n> We could try to be more careful, but it quickly became messy when I\n> tried. Here is an unfinished attempt of mine.\n\nSo yeah, I was hoping to avoid jumping into this rabbit hole of\nmessiness and just do the bare minimum to give us memory safety. But it\nseems nobody is quite happy with the result. :(\n\nLooking over what you wrote below, it seems pretty reasonable to me.\nWhat do you consider unfinished in it? I'm wondering if we should swap\nit into what my patch is doing (or do it on top if you prefer).\n\nAnother option is to scrap this approach entirely, and copy up until the\ntrailing newline into a separate buffer, NUL-terminate it, and parse\nfrom that buffer. That feels a little dirty to me, but I suspect it is\npretty performant in practice, and it pushes all of the complexity back\nonto strtol().\n\nAnother variant of that is: parse up to the trailing newline, making\nsure it's there, and then leave the rest of the code as-is. We know that\nstrtol() will do the right thing in that case, but it does mean we\ncannot use ASan's strict_string_checks (it would still yield a false\npositive, because it does not know we've checked for the newline).\n\n-Peff\n"},{"id":"531244","messageId":"xmqqms4buix0.fsf@gitster.g","threadId":"64468","inReplyTo":"20251124223023.GA2051672@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-24T23:09:47Z","receivedAt":"2025-11-24T23:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Looking over what you wrote below, it seems pretty reasonable to me.\n> What do you consider unfinished in it?\n\nTwo things I am unhappy about are that (1) parsing the digit\nsequence that represents abs(x) into unsigned int while catching\nwraparound and (2) checking if 'val' that has abs(x) would fit in a\nsigned int when 'negate' is applied. For both of them, there ought\nto be a better way to write, and perhaps there may be a clean way to\ndo both at the same time that is easier reason about.\n\n> Another option is to scrap this approach entirely, and copy up until the\n> trailing newline into a separate buffer, NUL-terminate it, and parse\n> from that buffer. That feels a little dirty to me, but I suspect it is\n> pretty performant in practice, and it pushes all of the complexity back\n> onto strtol().\n>\n> Another variant of that is: parse up to the trailing newline, making\n> sure it's there, and then leave the rest of the code as-is. We know that\n> strtol() will do the right thing in that case, but it does mean we\n> cannot use ASan's strict_string_checks (it would still yield a false\n> positive, because it does not know we've checked for the newline).\n\nOr perhaps introduce cache-tree-version-2 index extension. If there\nare other things we may want to fix while we are at it, that would\nbe a better way to spend our engineering resource, but I offhand do\nnot know of anything gravely lacking there that we may want to fix\n(there are little things like how the pathnames are sorted that I\nregret the way it was implemented, but that does not motivate me\nenough).\n\n\n"},{"id":"531303","messageId":"20251126150931.GC4143292@coredump.intra.peff.net","threadId":"64468","inReplyTo":"xmqqms4buix0.fsf@gitster.g","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-26T15:09:31Z","receivedAt":"2025-11-26T15:09:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2025 at 03:09:47PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Looking over what you wrote below, it seems pretty reasonable to me.\n> > What do you consider unfinished in it?\n> \n> Two things I am unhappy about are that (1) parsing the digit\n> sequence that represents abs(x) into unsigned int while catching\n> wraparound and (2) checking if 'val' that has abs(x) would fit in a\n> signed int when 'negate' is applied. For both of them, there ought\n> to be a better way to write, and perhaps there may be a clean way to\n> do both at the same time that is easier reason about.\n\nHmm, I thought both of those things were reasonably clever. The other\nobvious way to do it, AFAICT, is to used checked-operation intrinsics or\nadd unsigned_add_overflows() before every operation.\n\nIt is true that for the general case of: \"x = y + z\" or \"x = y * z\", you\ncannot determine overflow strictly from checking that x < y. But I think\ngiven that we know \"z\" must be small, it works in this case.\n\nIt looks like you merged what I had into 'next'. Where do you want to go\nfrom there? I am mostly content to let it be, but we can also try to\nreplace with something like your version. Or even, I guess, work on a\nglobal strntoi() that could be used everywhere, if we think it is robust\nenough. (Though technically that name is reserved by the standard, which\nis a shame, because that is really what this thing is).\n\n> Or perhaps introduce cache-tree-version-2 index extension. If there\n> are other things we may want to fix while we are at it, that would\n> be a better way to spend our engineering resource, but I offhand do\n> not know of anything gravely lacking there that we may want to fix\n> (there are little things like how the pathnames are sorted that I\n> regret the way it was implemented, but that does not motivate me\n> enough).\n\nI read your other email laying out the v2 concept, and I didn't disagree\nwith anything. It just feels like a bigger engineering effort and a\nbigger risk that the transition does not go as smoothly as we expect for\nsolving a very small implementation problem. But like you say, I do not\nhave a laundry list of cache-tree things I'd like to fix either. I think\nthe transition being worth it would depend on that kind of list.\n\n-Peff\n"},{"id":"531315","messageId":"xmqqldjsogip.fsf@gitster.g","threadId":"64468","inReplyTo":"20251126150931.GC4143292@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-26T17:22:38Z","receivedAt":"2025-11-26T17:22:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm, I thought both of those things were reasonably clever. The other\n> obvious way to do it, AFAICT, is to used checked-operation intrinsics or\n> add unsigned_add_overflows() before every operation.\n\nYup, but the thing is, I didn't want something \"clever\". I prefer\n\"clean and obvious\" if we add extra code for safety.\n\n> It is true that for the general case of: \"x = y + z\" or \"x = y * z\", you\n> cannot determine overflow strictly from checking that x < y. But I think\n> given that we know \"z\" must be small, it works in this case.\n>\n> It looks like you merged what I had into 'next'. Where do you want to go\n> from there? I am mostly content to let it be, but we can also try to\n> replace with something like your version.\n\nThat is my preference. While the topic is still in 'next', or after\nthe topic graduates to 'master'. Either is fine. And it is fine if\nsuch an update did not come, too. After all, this is to deal with\ncontents in a locally generated file (.git/index), so a maliciously\ncorrupt string that lack the expected whitespace character after the\ndigit string is a sign that you are trying to burn yourself and you\nhave only yourself to blame, isn't it? An attacker that can put\ngarbage in your .git/index has better ways to fool you by updating\nyour .git/config file that sits next to it. Or teach the sanitizer\nthat this code path is already OK somehow?\n\n> Or even, I guess, work on a\n> global strntoi() that could be used everywhere, if we think it is robust\n> enough. (Though technically that name is reserved by the standard, which\n> is a shame, because that is really what this thing is).\n\nWell, we already use plenty of names beginning with 'str' followed\nby a lowercase letter, like strbuf_foo() and string_list_init().\n"},{"id":"531463","messageId":"20251130131351.GA198697@coredump.intra.peff.net","threadId":"64468","inReplyTo":"xmqqldjsogip.fsf@gitster.g","subject":"[PATCH 0/4] more robust functions for parsing int from buf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:13:51Z","receivedAt":"2025-11-30T13:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 26, 2025 at 09:22:38AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Hmm, I thought both of those things were reasonably clever. The other\n> > obvious way to do it, AFAICT, is to used checked-operation intrinsics or\n> > add unsigned_add_overflows() before every operation.\n> \n> Yup, but the thing is, I didn't want something \"clever\". I prefer\n> \"clean and obvious\" if we add extra code for safety.\n\nYeah, that's fair. It turns out that one half of that is easy: checking\nfor overflow as we compute the number). And one half is hard. If you\ndon't assume a twos-complement style range where the \"min = -max - 1\",\nthen you are stuck using INT_MIN. Which is OK for \"int\", but not for\narbitrary types. We already make the same assumption in git_parse_int(),\netc.\n\nSo I went with that approach here, but it is at least documented\nclearly.\n\n> > It looks like you merged what I had into 'next'. Where do you want to go\n> > from there? I am mostly content to let it be, but we can also try to\n> > replace with something like your version.\n> \n> That is my preference. While the topic is still in 'next', or after\n> the topic graduates to 'master'. Either is fine. And it is fine if\n> such an update did not come, too. After all, this is to deal with\n> contents in a locally generated file (.git/index), so a maliciously\n> corrupt string that lack the expected whitespace character after the\n> digit string is a sign that you are trying to burn yourself and you\n> have only yourself to blame, isn't it? An attacker that can put\n> garbage in your .git/index has better ways to fool you by updating\n> your .git/config file that sits next to it. Or teach the sanitizer\n> that this code path is already OK somehow?\n\nYeah, I agree the stakes are low here. Though they were somewhat low to\nbegin with for the same reason! But I was grossed out enough by the\nwhole thing that I tried to put together a decent helper for parsing\nintegers from buffers, and converted both sites here.\n\nI suspect it could be used in other places, too, but I didn't convert\nany.\n\n> > Or even, I guess, work on a\n> > global strntoi() that could be used everywhere, if we think it is robust\n> > enough. (Though technically that name is reserved by the standard, which\n> > is a shame, because that is really what this thing is).\n> \n> Well, we already use plenty of names beginning with 'str' followed\n> by a lowercase letter, like strbuf_foo() and string_list_init().\n\nIn the end it was sufficiently different from strtoi() that I decided\nnot to use that name. It was but one of many bike-sheddable decisions,\nwhich I tried to document. So I guess let the flaming commence. ;)\n\nThis is built on top of jk/asan-bonanza.\n\n [1/4]: parse: prefer bool to int for boolean returns\n [2/4]: parse: add functions for parsing from non-string buffers\n [3/4]: cache-tree: use parse_int_from_buf()\n [4/4]: fsck: use parse_unsigned_from_buf() for parsing timestamp\n\n Makefile | 1 +\n cache-tree.c | 28 ++-----\n compat/posix.h | 2 +\n fsck.c | 20 +----\n parse.c | 162 +++++++++++++++++++++++++++++--------\n parse.h | 31 +++++--\n t/meson.build | 1 +\n t/unit-tests/u-parse-int.c | 98 ++++++++++++++++++++++\n 8 files changed, 263 insertions(+), 80 deletions(-)\n create mode 100644 t/unit-tests/u-parse-int.c\n\n-Peff\n"},{"id":"531464","messageId":"20251130131441.GA199335@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251130131351.GA198697@coredump.intra.peff.net","subject":"[PATCH 1/4] parse: prefer bool to int for boolean returns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:14:41Z","receivedAt":"2025-11-30T13:14:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the integer parsing functions in parse.[ch] return an int that is\n\"0\" for failure or \"1\" for success. Since most of the other functions in\nGit use \"0\" for success and \"-1\" for failure, this can be confusing.\nLet's switch the return types to bool to make it clear that we are using\nthis other convention. Callers should not need to update at all.\n\nSigned-off-by: Jeff King \n---\nObviously not strictly necessary for this series, but I think a good\nidea regardless of the rest of it.\n\n parse.c | 66 ++++++++++++++++++++++++++++-----------------------------\n parse.h | 14 ++++++------\n 2 files changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/parse.c b/parse.c\nindex 48313571aa..f626846def 100644\n--- a/parse.c\n+++ b/parse.c\n@@ -15,7 +15,7 @@ static uintmax_t get_unit_factor(const char *end)\n \treturn 0;\n }\n \n-int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n+bool git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n {\n \tif (value && *value) {\n \t\tchar *end;\n@@ -28,30 +28,30 @@ int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\terrno = 0;\n \t\tval = strtoimax(value, &end, 0);\n \t\tif (errno == ERANGE)\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\tif (end == value) {\n \t\t\terrno = EINVAL;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tif ((val < 0 && (-max - 1) / factor > val) ||\n \t\t (val > 0 && max / factor < val)) {\n \t\t\terrno = ERANGE;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tval *= factor;\n \t\t*ret = val;\n-\t\treturn 1;\n+\t\treturn true;\n \t}\n \terrno = EINVAL;\n-\treturn 0;\n+\treturn false;\n }\n \n-int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n+bool git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n {\n \tif (value && *value) {\n \t\tchar *end;\n@@ -61,97 +61,97 @@ int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\t/* negative values would be accepted by strtoumax */\n \t\tif (strchr(value, '-')) {\n \t\t\terrno = EINVAL;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\terrno = 0;\n \t\tval = strtoumax(value, &end, 0);\n \t\tif (errno == ERANGE)\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\tif (end == value) {\n \t\t\terrno = EINVAL;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tif (unsigned_mult_overflows(factor, val) ||\n \t\t factor * val > max) {\n \t\t\terrno = ERANGE;\n-\t\t\treturn 0;\n+\t\t\treturn false;\n \t\t}\n \t\tval *= factor;\n \t\t*ret = val;\n-\t\treturn 1;\n+\t\treturn true;\n \t}\n \terrno = EINVAL;\n-\treturn 0;\n+\treturn false;\n }\n \n-int git_parse_int(const char *value, int *ret)\n+bool git_parse_int(const char *value, int *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))\n-\t\treturn 0;\n+\t\treturn false;\n \t*ret = tmp;\n-\treturn 1;\n+\treturn true;\n }\n \n-int git_parse_int64(const char *value, int64_t *ret)\n+bool git_parse_int64(const char *value, int64_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))\n-\t\treturn 0;\n+\t\treturn false;\n \t*ret = tmp;\n-\treturn 1;\n+\treturn true;\n }\n \n-int git_parse_ulong(const char *value, unsigned long *ret)\n+bool git_parse_ulong(const char *value, unsigned long *ret)\n {\n \tuintmax_t tmp;\n \tif (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(long)))\n-\t\treturn 0;\n+\t\treturn false;\n \t*ret = tmp;\n-\treturn 1;\n+\treturn true;\n }\n \n-int git_parse_ssize_t(const char *value, ssize_t *ret)\n+bool git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\n-\t\treturn 0;\n+\t\treturn false;\n \t*ret = tmp;\n-\treturn 1;\n+\treturn true;\n }\n \n-int git_parse_double(const char *value, double *ret)\n+bool git_parse_double(const char *value, double *ret)\n {\n \tchar *end;\n \tdouble val;\n \tuintmax_t factor;\n \n \tif (!value || !*value) {\n \t\terrno = EINVAL;\n-\t\treturn 0;\n+\t\treturn false;\n \t}\n \n \terrno = 0;\n \tval = strtod(value, &end);\n \tif (errno == ERANGE)\n-\t\treturn 0;\n+\t\treturn false;\n \tif (end == value) {\n \t\terrno = EINVAL;\n-\t\treturn 0;\n+\t\treturn false;\n \t}\n \tfactor = get_unit_factor(end);\n \tif (!factor) {\n \t\terrno = EINVAL;\n-\t\treturn 0;\n+\t\treturn false;\n \t}\n \tval *= factor;\n \t*ret = val;\n-\treturn 1;\n+\treturn true;\n }\n \n int git_parse_maybe_bool_text(const char *value)\ndiff --git a/parse.h b/parse.h\nindex ea32de9a91..f80cc5b9fd 100644\n--- a/parse.h\n+++ b/parse.h\n@@ -1,13 +1,13 @@\n #ifndef PARSE_H\n #define PARSE_H\n \n-int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);\n-int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);\n-int git_parse_ssize_t(const char *, ssize_t *);\n-int git_parse_ulong(const char *, unsigned long *);\n-int git_parse_int(const char *value, int *ret);\n-int git_parse_int64(const char *value, int64_t *ret);\n-int git_parse_double(const char *value, double *ret);\n+bool git_parse_signed(const char *value, intmax_t *ret, intmax_t max);\n+bool git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);\n+bool git_parse_ssize_t(const char *, ssize_t *);\n+bool git_parse_ulong(const char *, unsigned long *);\n+bool git_parse_int(const char *value, int *ret);\n+bool git_parse_int64(const char *value, int64_t *ret);\n+bool git_parse_double(const char *value, double *ret);\n \n /**\n * Same as `git_config_bool`, except that it returns -1 on error rather\n-- \n2.52.0.413.gf695cdb9bd\n\n"},{"id":"531465","messageId":"20251130131537.GB199335@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251130131351.GA198697@coredump.intra.peff.net","subject":"[PATCH 2/4] parse: add functions for parsing from non-string buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:15:37Z","receivedAt":"2025-11-30T13:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have a buffer that is not NUL-terminated but want to parse an\ninteger, there aren't many good options. If you use strtol() and\nfriends, you risk running off the end of the buffer if there is no\nnon-digit terminating character. And even if you carefully make sure\nthat there is such a character, ASan's strict-string-check mode will\nstill complain.\n\nYou can copy bytes into a temporary buffer, terminate it, and then call\nstrtol(), but doing so adds some pitfalls (like making sure you soak up\nwhitespace and leading +/- signs, and reporting overflow for overly long\ninput). Or you can hand-parse the digits, but then you need to take some\ncare to handle overflow (and again, whitespace and +/- signs).\n\nThese things aren't impossible to do right, but it's error-prone to have\nto do them in every spot that wants to do such parsing. So let's add\nsome functions which can be used across the code base.\n\nThere are a few choices regarding the interface and the implementation.\n\nFirst, the implementation:\n\n - I went with with parsing the digits (rather than buffering and\n passing to libc functions). It ends up being a similar amount of\n code because we have to do some parsing either way. And likewise\n overflow detection depends on the exact type the caller wants, so we\n either have to do it by hand or write a separate wrapper for\n strtol(), strtoumax(), and so on.\n\n - Unsigned overflow detection is done using the same techniques as in\n unsigned_add_overflows(), etc. We can't use those macros directly\n because our core function is type-agnostic (so the caller passes in\n the max value, rather than us deriving it on the fly). This is\n similar to how git_parse_int(), etc, work.\n\n - Signed overflow detection assumes that we can express a negative\n value with magnitude one larger than our maximum positive value\n (e.g., -128..127 for a signed 8-bit value). I doubt this is\n guaranteed by the standard, but it should hold in practice, and we\n make the same assumption in git_parse_int(), etc. The nice thing\n about this is that we can derive the range from the number of bits\n in the type. For ints, you obviously could use INT_MIN..INT_MAX, but\n for an arbitrary type, we can use maximum_signed_value_of_type().\n\n - I didn't bother with handling bases other than 10. It would\n complicate the code, and I suspect it won't be needed. We could\n probably retro-fit it later without too much work, if need be.\n\nFor the interface:\n\n - What do we call it? We have git_parse_int() and friends, which aim\n to make parsing less error-prone. And in some ways, these are just\n buffer (rather than string) versions of those functions. But not\n entirely. Those functions are aimed at parsing a single user-facing\n value. So they accept a unit prefix (e.g., \"10k\"), which we won't\n always want. And they insist that the whole string is consumed\n (rather than passing back an \"end\" pointer).\n\n We also have strtol_i() and strtoul_ui() wrappers, which try to make\n error handling simpler (especially around overflow), but mostly\n behave like their libc counterparts. These also don't pass out an\n end pointer, though.\n\n So I started a new namespace, \"parse__from_buf\".\n\n - Like those other functions above, we use an out-parameter to store\n the result, which lets us return an error code directly. This avoids\n the complicated errno dance for detecting overflow that you get with\n strtol().\n\n What should the error code look like? git_parse_int() uses a bool\n for success/failure. But strtol_ui() uses the syscall-like \"0 is\n success, -1 is error\" convention.\n\n I went with the bool approach here. Since the names are closest to\n those functions, I thought it would cause the least confusion.\n\n - Unlike git_parse_signed() and friends, we do not insist that the\n entire buffer be consumed. For parsing a specific standalone string\n that makes sense, but within an unterminated buffer you are much\n more likely to be parsing multiple fields from a larger data set.\n\n We pass out an \"end\" pointer the same way strtol() does. Another\n option is to accept the input as an in-out parameter and advance the\n pointer ourselves (and likewise shrink the length pointer). That\n would let you do something like:\n\n if (!parse_int_from_buf(&p, &len, &out))\n return error(...);\n /* \"p\" and \"len\" were adjusted automatically */\n if (!len || *p++ != ' ')\n return error(...);\n\n That saves a few lines of code in some spots, but requires a few\n more in others (depending on whether the caller has a length in the\n first place or is using an end pointer). Of the two callers I intend\n to immediately convert, we have one of each type!\n\n I went with the strtol() approach as flexible and time-tested.\n\n - We could likewise take the input buffer as two pointers (start and\n end) rather than a pointer and a length. That again makes life\n easier for some callers and harder for others. I stuck with pointer\n and length as the more usual interface.\n\n - What happens when a caller passes in a NULL end pointer? This is\n allowed by strtol(). But I think it's often a sign of a lurking bug,\n because there's no way to know how much was consumed (and even if a\n caller wants to assume everything is consumed, you have no way to\n verify it). So it is simply an error in this interface (you'd get a\n segfault).\n\n I am tempted to say that if the end pointer is NULL the functions\n could confirm that the entire buffer was consumed, as a convenience.\n But that felt a bit magical and surprising.\n\nLike git_parse_*(), there is a generic signed/unsigned helper, and then\nwe can add type-specific helpers on top. I've added an int helper here\nto start, and we'll add more as we convert callers.\n\nSigned-off-by: Jeff King \n---\nSorry for the long message, but I tried to lay out my thinking for all\nof it, since there were a lot of arbitrary decisions.\n\n Makefile | 1 +\n parse.c | 96 +++++++++++++++++++++++++++++++++++++\n parse.h | 17 +++++++\n t/meson.build | 1 +\n t/unit-tests/u-parse-int.c | 98 ++++++++++++++++++++++++++++++++++++++\n 5 files changed, 213 insertions(+)\n create mode 100644 t/unit-tests/u-parse-int.c\n\ndiff --git a/Makefile b/Makefile\nindex 237b56fc9d..751bd40a9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1510,6 +1510,7 @@ CLAR_TEST_SUITES += u-mem-pool\n CLAR_TEST_SUITES += u-oid-array\n CLAR_TEST_SUITES += u-oidmap\n CLAR_TEST_SUITES += u-oidtree\n+CLAR_TEST_SUITES += u-parse-int\n CLAR_TEST_SUITES += u-prio-queue\n CLAR_TEST_SUITES += u-reftable-basics\n CLAR_TEST_SUITES += u-reftable-block\ndiff --git a/parse.c b/parse.c\nindex f626846def..1dcbcf64a1 100644\n--- a/parse.c\n+++ b/parse.c\n@@ -209,3 +209,99 @@ unsigned long git_env_ulong(const char *k, unsigned long val)\n \t\tdie(_(\"failed to parse %s\"), k);\n \treturn val;\n }\n+\n+/*\n+ * Helper that handles both signed/unsigned cases. If \"negate\" is NULL,\n+ * negative values are disallowed. If not NULL and the input is negative,\n+ * the value is range-checked but the caller is responsible for actually doing\n+ * the negatiion. You probably don't want to use this! Use one of\n+ * parse_signed_from_buf() or parse_unsigned_from_buf() below.\n+ */\n+static bool parse_from_buf_internal(const char *buf, size_t len,\n+\t\t\t\t const char **ep, bool *negate,\n+\t\t\t\t uintmax_t *ret, uintmax_t max)\n+{\n+\tconst char *end = buf + len;\n+\tuintmax_t val = 0;\n+\n+\twhile (buf < end && isspace(*buf))\n+\t\tbuf++;\n+\n+\tif (negate)\n+\t\t*negate = false;\n+\tif (buf < end && *buf == '-') {\n+\t\tif (!negate) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn false;\n+\t\t}\n+\t\tbuf++;\n+\t\t*negate = true;\n+\t\t/* Assume negative range is always one larger than positive. */\n+\t\tmax = max + 1;\n+\t} else if (buf < end && *buf == '+') {\n+\t\tbuf++;\n+\t}\n+\n+\tif (buf == end || !isdigit(*buf)) {\n+\t\terrno = EINVAL;\n+\t\treturn false;\n+\t}\n+\n+\twhile (buf < end && isdigit(*buf)) {\n+\t\tint digit = *buf - '0';\n+\n+\t\tif (val > max / 10) {\n+\t\t\terrno = ERANGE;\n+\t\t\treturn false;\n+\t\t}\n+\t\tval *= 10;\n+\t\tif (val > max - digit) {\n+\t\t\terrno = ERANGE;\n+\t\t\treturn false;\n+\t\t}\n+\t\tval += digit;\n+\n+\t\tbuf++;\n+\t}\n+\n+\t*ep = buf;\n+\t*ret = val;\n+\treturn true;\n+}\n+\n+bool parse_unsigned_from_buf(const char *buf, size_t len, const char **ep,\n+\t\t\t uintmax_t *ret, uintmax_t max)\n+{\n+\treturn parse_from_buf_internal(buf, len, ep, NULL, ret, max);\n+}\n+\n+bool parse_signed_from_buf(const char *buf, size_t len, const char **ep,\n+\t\t\t intmax_t *ret, intmax_t max)\n+{\n+\tuintmax_t u_ret;\n+\tbool negate;\n+\n+\tif (!parse_from_buf_internal(buf, len, ep, &negate, &u_ret, max))\n+\t\treturn false;\n+\t/*\n+\t * Range already checked internally, but we must apply negation\n+\t * ourselves since only we have the signed integer type.\n+\t */\n+\tif (negate) {\n+\t\t*ret = u_ret;\n+\t\t*ret = -*ret;\n+\t} else {\n+\t\t*ret = u_ret;\n+\t}\n+\treturn true;\n+}\n+\n+bool parse_int_from_buf(const char *buf, size_t len, const char **ep, int *ret)\n+{\n+\tintmax_t tmp;\n+\tif (!parse_signed_from_buf(buf, len, ep, &tmp,\n+\t\t\t\t maximum_signed_value_of_type(int)))\n+\t\treturn false;\n+\t*ret = tmp;\n+\treturn true;\n+}\ndiff --git a/parse.h b/parse.h\nindex f80cc5b9fd..53663c8939 100644\n--- a/parse.h\n+++ b/parse.h\n@@ -19,4 +19,21 @@ int git_parse_maybe_bool_text(const char *value);\n int git_env_bool(const char *, int);\n unsigned long git_env_ulong(const char *, unsigned long);\n \n+/*\n+ * These functions parse an integer from a buffer that does not need to be\n+ * NUL-terminated. They return true on success, or false if no integer is found\n+ * (in which case errno is set to EINVAL) or if the integer is out of the\n+ * allowable range (in which case errno is ERANGE).\n+ *\n+ * You must pass in a non-NULL value for \"ep\", which returns a pointer to the\n+ * next character in the buf (similar to strtol(), etc).\n+ *\n+ * These functions always parse in base 10 (and do not allow input like \"0xff\"\n+ * to switch to base 16). They do not allow unit suffixes like git_parse_int(),\n+ * above.\n+ */\n+bool parse_unsigned_from_buf(const char *buf, size_t len, const char **ep, uintmax_t *ret, uintmax_t max);\n+bool parse_signed_from_buf(const char *buf, size_t len, const char **ep, intmax_t *ret, intmax_t max);\n+bool parse_int_from_buf(const char *buf, size_t len, const char **ep, int *ret);\n+\n #endif /* PARSE_H */\ndiff --git a/t/meson.build b/t/meson.build\nindex 7c994d4643..1289614545 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -8,6 +8,7 @@ clar_test_suites = [\n 'unit-tests/u-oid-array.c',\n 'unit-tests/u-oidmap.c',\n 'unit-tests/u-oidtree.c',\n+ 'unit-tests/u-parse-int.c',\n 'unit-tests/u-prio-queue.c',\n 'unit-tests/u-reftable-basics.c',\n 'unit-tests/u-reftable-block.c',\ndiff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c\nnew file mode 100644\nindex 0000000000..a1601bb16b\n--- /dev/null\n+++ b/t/unit-tests/u-parse-int.c\n@@ -0,0 +1,98 @@\n+#include \"unit-test.h\"\n+#include \"parse.h\"\n+\n+static void check_int(const char *buf, size_t len,\n+\t\t size_t expect_ep_ofs, int expect_errno,\n+\t\t int expect_result)\n+{\n+\tconst char *ep;\n+\tint result;\n+\tbool ok = parse_int_from_buf(buf, len, &ep, &result);\n+\n+\tif (expect_errno) {\n+\t\tcl_assert(!ok);\n+\t\tcl_assert_equal_i(expect_errno, errno);\n+\t\treturn;\n+\t}\n+\n+\tcl_assert(ok);\n+\tcl_assert_equal_i(expect_result, result);\n+\tcl_assert_equal_i(expect_ep_ofs, ep - buf);\n+}\n+\n+static void check_int_str(const char *buf, size_t ofs, int err, int res)\n+{\n+\tcheck_int(buf, strlen(buf), ofs, err, res);\n+}\n+\n+static void check_int_full(const char *buf, int res)\n+{\n+\tcheck_int_str(buf, strlen(buf), 0, res);\n+}\n+\n+static void check_int_err(const char *buf, int err)\n+{\n+\tcheck_int(buf, strlen(buf), 0, err, 0);\n+}\n+\n+void test_parse_int__basic(void)\n+{\n+\tcl_invoke(check_int_full(\"0\", 0));\n+\tcl_invoke(check_int_full(\"11\", 11));\n+\tcl_invoke(check_int_full(\"-23\", -23));\n+\tcl_invoke(check_int_full(\"+23\", 23));\n+\n+\tcl_invoke(check_int_str(\" 31337 \", 7, 0, 31337));\n+\n+\tcl_invoke(check_int_err(\" garbage\", EINVAL));\n+\tcl_invoke(check_int_err(\"\", EINVAL));\n+\tcl_invoke(check_int_err(\"-\", EINVAL));\n+\n+\tcl_invoke(check_int(\"123\", 2, 2, 0, 12));\n+}\n+\n+void test_parse_int__range(void)\n+{\n+\t/*\n+\t * These assume a 32-bit int. We could avoid that with some\n+\t * conditionals, but it's probably better for the test to\n+\t * fail noisily and we can decide how to handle it then.\n+\t */\n+\tcl_invoke(check_int_full(\"2147483647\", 2147483647));\n+\tcl_invoke(check_int_err(\"2147483648\", ERANGE));\n+\tcl_invoke(check_int_full(\"-2147483647\", -2147483647));\n+\tcl_invoke(check_int_full(\"-2147483648\", -2147483648));\n+\tcl_invoke(check_int_err(\"-2147483649\", ERANGE));\n+}\n+\n+static void check_unsigned(const char *buf, uintmax_t max,\n+\t\t\t int expect_errno, uintmax_t expect_result)\n+{\n+\tconst char *ep;\n+\tuintmax_t result;\n+\tbool ok = parse_unsigned_from_buf(buf, strlen(buf), &ep, &result, max);\n+\n+\tif (expect_errno) {\n+\t\tcl_assert(!ok);\n+\t\tcl_assert_equal_i(expect_errno, errno);\n+\t\treturn;\n+\t}\n+\n+\tcl_assert(ok);\n+\tcl_assert_equal_s(ep, \"\");\n+\t/*\n+\t * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro\n+\t * casts to int under the hood, corrupting the values.\n+\t */\n+\tclar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,\n+\t\t\t CLAR_CURRENT_LINE,\n+\t\t\t \"expect_result != result\", 1,\n+\t\t\t \"%\"PRIuMAX, expect_result, result);\n+}\n+\n+void test_parse_int__unsigned(void)\n+{\n+\tcl_invoke(check_unsigned(\"4294967295\", UINT_MAX, 0, 4294967295U));\n+\tcl_invoke(check_unsigned(\"1053\", 1000, ERANGE, 0));\n+\tcl_invoke(check_unsigned(\"-17\", UINT_MAX, EINVAL, 0));\n+}\n-- \n2.52.0.413.gf695cdb9bd\n\n"},{"id":"531466","messageId":"20251130131551.GC199335@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251130131351.GA198697@coredump.intra.peff.net","subject":"[PATCH 3/4] cache-tree: use parse_int_from_buf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:15:51Z","receivedAt":"2025-11-30T13:15:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In c4c9089584 (cache-tree: avoid strtol() on non-string buffer,\n2025-11-18) we wrote an ad-hoc integer parser which did not detect\noverflow. This wasn't too big a problem, since the original use of\nstrtol() did not do so either. But now that we have a more robust\nparsing function, let's use that. It reduces the amount of code and\nshould catch more cases of malformed entries.\n\nI kept our local parse_int() wrapper here, since it handles management\nof our ptr/len pair (rather than doing it inline in the entry parser of\nread_one()).\n\nSigned-off-by: Jeff King \n---\n cache-tree.c | 28 +++++-----------------------\n 1 file changed, 5 insertions(+), 23 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2d8947b518..f8fb290443 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -16,6 +16,7 @@\n #include \"promisor-remote.h\"\n #include \"trace.h\"\n #include \"trace2.h\"\n+#include \"parse.h\"\n \n #ifndef DEBUG_CACHE_TREE\n #define DEBUG_CACHE_TREE 0\n@@ -550,32 +551,13 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)\n \n static int parse_int(const char **ptr, unsigned long *len_p, int *out)\n {\n-\tconst char *s = *ptr;\n-\tunsigned long len = *len_p;\n-\tint ret = 0;\n-\tint sign = 1;\n-\n-\twhile (len && *s == '-') {\n-\t\tsign *= -1;\n-\t\ts++;\n-\t\tlen--;\n-\t}\n-\n-\twhile (len) {\n-\t\tif (!isdigit(*s))\n-\t\t\tbreak;\n-\t\tret *= 10;\n-\t\tret += *s - '0';\n-\t\ts++;\n-\t\tlen--;\n-\t}\n+\tconst char *ep;\n \n-\tif (s == *ptr)\n+\tif (!parse_int_from_buf(*ptr, *len_p, &ep, out))\n \t\treturn -1;\n \n-\t*ptr = s;\n-\t*len_p = len;\n-\t*out = sign * ret;\n+\t*len_p -= ep - *ptr;\n+\t*ptr = ep;\n \treturn 0;\n }\n \n-- \n2.52.0.413.gf695cdb9bd\n\n"},{"id":"531467","messageId":"20251130131602.GD199335@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251130131351.GA198697@coredump.intra.peff.net","subject":"[PATCH 4/4] fsck: use parse_unsigned_from_buf() for parsing timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:16:02Z","receivedAt":"2025-11-30T13:16:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 5a993593b2 (fsck: avoid parse_timestamp() on buffer that isn't\nNUL-terminated, 2025-11-18), we added a wrapper that copies the\ntimestamp into a buffer before calling parse_timestamp().\n\nNow that we have a more robust helper for parsing from a buffer, we can\ndrop our wrapper and switch to that. We could just do so inline, but the\nchoice of \"unsigned\" vs \"signed\" depends on the typedef of timestamp_t.\nSo we'll wrap that in a macro that is defined alongside the rest of the\ntimestamp abstraction.\n\nThe resulting function is almost a drop-in replacement, but the new\ninterface means we need to hold the result in a separate timestamp_t,\nrather than returning it directly from one function into the parameter\nof another. The old one did still detect overflow errors by returning\nTIME_MAX, since date_overflows() checks for that, but now we'll see it\nmore directly from the return of parse_timestamp_from_buf(). The\nbehavior should be the same.\n\nSigned-off-by: Jeff King \n---\n compat/posix.h | 2 ++\n fsck.c | 20 +++-----------------\n 2 files changed, 5 insertions(+), 17 deletions(-)\n\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 067a00f33b..d2dbc3e2a5 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -253,6 +253,8 @@ char *gitdirname(char *);\n typedef uintmax_t timestamp_t;\n #define PRItime PRIuMAX\n #define parse_timestamp strtoumax\n+#define parse_timestamp_from_buf(buf, len, ep, result) \\\n+\tparse_unsigned_from_buf((buf), (len), (ep), (result), TIME_MAX)\n #define TIME_MAX UINTMAX_MAX\n #define TIME_MIN 0\n \ndiff --git a/fsck.c b/fsck.c\nindex 8e8083e7c6..68a23ae628 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -860,28 +860,13 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n-static timestamp_t parse_timestamp_from_buf(const char **start, const char *end)\n-{\n-\tconst char *p = *start;\n-\tchar buf[24]; /* big enough for 2^64 */\n-\tsize_t i = 0;\n-\n-\twhile (p < end && isdigit(*p)) {\n-\t\tif (i >= ARRAY_SIZE(buf) - 1)\n-\t\t\treturn TIME_MAX;\n-\t\tbuf[i++] = *p++;\n-\t}\n-\tbuf[i] = '\\0';\n-\t*start = p;\n-\treturn parse_timestamp(buf, NULL, 10);\n-}\n-\n static int fsck_ident(const char **ident, const char *ident_end,\n \t\t const struct object_id *oid, enum object_type type,\n \t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n \tconst char *nl;\n+\ttimestamp_t timestamp;\n \n \tnl = memchr(p, '\\n', ident_end - p);\n \tif (!nl)\n@@ -933,7 +918,8 @@ static int fsck_ident(const char **ident, const char *ident_end,\n \t\t\t \"invalid author/committer line - bad date\");\n \tif (*p == '0' && p[1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n-\tif (date_overflows(parse_timestamp_from_buf(&p, ident_end)))\n+\tif (!parse_timestamp_from_buf(p, ident_end - p, &p, ×tamp) ||\n+\t date_overflows(timestamp))\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n \tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n-- \n2.52.0.413.gf695cdb9bd\n"},{"id":"531468","messageId":"20251130134625.GA199421@coredump.intra.peff.net","threadId":"64468","inReplyTo":"20251130131537.GB199335@coredump.intra.peff.net","subject":"my complaints with clar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-30T13:46:25Z","receivedAt":"2025-11-30T13:46:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"> --- /dev/null\n> +++ b/t/unit-tests/u-parse-int.c\n\nThis was my first time writing unit tests with clar, so I thought I'd\ndocument a few rough edges I found. It's possible I'm holding it wrong\nin some areas.\n\n> +static void check_int(const char *buf, size_t len,\n> +\t\t size_t expect_ep_ofs, int expect_errno,\n> +\t\t int expect_result)\n> +{\n> +\tconst char *ep;\n> +\tint result;\n> +\tbool ok = parse_int_from_buf(buf, len, &ep, &result);\n> +\n> +\tif (expect_errno) {\n> +\t\tcl_assert(!ok);\n> +\t\tcl_assert_equal_i(expect_errno, errno);\n> +\t\treturn;\n> +\t}\n> +\n> +\tcl_assert(ok);\n> +\tcl_assert_equal_i(expect_result, result);\n> +\tcl_assert_equal_i(expect_ep_ofs, ep - buf);\n> +}\n\nThe error messages I got on failure from this function were not super\ninformative. Naively, if you do something like:\n\n check_int_full(\"0\", 0);\n check_int_full(\"11\", 11);\n check_int_full(\"-23\", -23);\n check_int_full(\"+23\", 23);\n\nand it fails, you'll get not much beyond \"expected ok, but it's not\ntrue\" with a line number in the helper, but no idea which input failed.\nSo OK, we have cl_invoke for that, and:\n\n cl_invoke(check_int_full(\"0\", 0));\n cl_invoke(check_int_full(\"11\", 11));\n cl_invoke(check_int_full(\"-23\", -23));\n cl_invoke(check_int_full(\"+23\", 23));\n\ngives you the line number in the caller. Better, but there's a lot of\ncross-referencing the line numbers (plus sprinkling cl_invoke everywhere\nis ugly).\n\nWhat I really would have liked is some notion of \"context\". If the\nhelper could have done:\n\n cl_context(\"input: %.*s\", (int)len, buf);\n\nor similar, and failed assertions print that context, then that would\nhave made the failing part of the test easy to see, even without using\ncl_invoke() at all.\n\nAlternatively, I kind of wonder if cl_invoke() could just stringify the\nentire macro argument and shove that into the context. That helps for:\n\n cl_invoke(check_int_full(\"11\", 11));\n\nthough not if parameters are opaque in that line, like:\n\n cl_invoke(check_int_full(str, expect));\n\nBut I think boilerplate-saving helpers tend to be written more like the\nfirst way.\n\nIn a more general sense, what I'd really have loved is an automatic\nbacktrace, but I suspect getting a readable one is impossible. Even if\nwe knew the called function and the parameters, a generic backtracer\ncannot know the meaning of \"buf\" and \"len\" enough to show what was in\nthe buffer.\n\nAnd of course the more obvious way to avoid that is to break this:\n\n> +void test_parse_int__basic(void)\n> +{\n> +\tcl_invoke(check_int_full(\"0\", 0));\n> +\tcl_invoke(check_int_full(\"11\", 11));\n> +\tcl_invoke(check_int_full(\"-23\", -23));\n> +\tcl_invoke(check_int_full(\"+23\", 23));\n> +\tcl_invoke(check_int_str(\" 31337 \", 7, 0, 31337));\n> +\n> +\tcl_invoke(check_int_err(\" garbage\", EINVAL));\n> +\tcl_invoke(check_int_err(\"\", EINVAL));\n> +\tcl_invoke(check_int_err(\"-\", EINVAL));\n> +\n> +\tcl_invoke(check_int(\"123\", 2, 2, 0, 12));\n> +}\n\ninto a series of nine separate tests, each of which gets a name. But\neach of those tests is at least five lines of boilerplate, which sucks\n(plus you have to come up with syntactically valid C names for them).\n\n> +\t/*\n> +\t * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro\n> +\t * casts to int under the hood, corrupting the values.\n> +\t */\n> +\tclar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,\n> +\t\t\t CLAR_CURRENT_LINE,\n> +\t\t\t \"expect_result != result\", 1,\n> +\t\t\t \"%\"PRIuMAX, expect_result, result);\n> +}\n\nThis was an exciting bug to track down. If you use i_fmt() here, you get\nsome neat undefined behavior. It worked for gcc, but failed with clang\n(but only with -O2!).\n\nObviously this was me using it wrong, and the \"i\" in the macro should\nhave been a hint. But this invocation is kind of ugly, with the explicit\nmentions of internal CLAR variables. clar__assert_equal() understands\nPRIuMAX as a comparator, but there doesn't appear to be any macro to use\nit nicely.\n\nShould there be a generic cl_assert_equal() that fills in the first\nfew parameters but is otherwise type-agnostic?\n\nIt also looked error-prone to me that if you pass in an unknown format\nspecifier, clar__assert_equal() will assume you want integers. So a\ntypo, or using an unknown-but-equivalent specifier will give you weird\nundefined behavior bugs. These are just tests, so we can perhaps a bit\nmore loose, but these kinds of things can be hard to track down\n(especially if they trigger only in certain compiler combos via CI,\nwhich is what happened to me).\n\nAnd that brings me to my final complaint. ;)\n\nWhen the unit-tests fail in CI, you get very little useful feedback,\nbecause the output is eaten by \"prove\", and the unit tests don't\nunderstand --verbose-log at all. And then to make it more exciting, the\noutput that clar produces actually chokes prove. For example, if I do\nthis:\n\ndiff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c\nindex a1601bb16b..da706d5840 100644\n--- a/t/unit-tests/u-parse-int.c\n+++ b/t/unit-tests/u-parse-int.c\n@@ -38,7 +38,7 @@ static void check_int_err(const char *buf, int err)\n void test_parse_int__basic(void)\n {\n \tcl_invoke(check_int_full(\"0\", 0));\n-\tcl_invoke(check_int_full(\"11\", 11));\n+\tcl_invoke(check_int_full(\"11\", 10));\n \tcl_invoke(check_int_full(\"-23\", -23));\n \tcl_invoke(check_int_full(\"+23\", 23));\n \n\nthen running t/unit-tests/bin/unit-tests produces:\n\n \n # start of suite 10: parse_int\n not ok 59 - parse_int::basic\n ---\n reason: |\n expect_result != result\n 10 != 11\n at:\n file: 't/unit-tests/u-parse-int.c'\n line: 41\n function: 'test_parse_int__basic'\n ---\n\nOK, but \"prove t/unit-tests/bin/unit-tests\" gives me:\n\n t/unit-tests/bin/unit-tests .. Failed 1/59 subtests\n \n Test Summary Report\n -------------------\n t/unit-tests/bin/unit-tests (Wstat: (none) Tests: 59 Failed: 1)\n Failed test: 59\n Parse errors: Badly formed hash line: '---' at /usr/share/perl/5.40/TAP/Parser/YAMLish/Reader.pm line 244.\n\nYuck. It actually does have what I need (that test 59 was the failure),\nso the extra parse error is mostly a red herring (though it does prevent\nus finding any further failures). I think in TAP that arbitrary output\nis supposed to be prefixed with a \"#\". In test-lib.sh, we solve this by\nonly allowing \"--verbose-log\", not regular \"-v\", under a TAP harness.\n\nI kind of wonder if we should have t0011-unit-tests.sh that simply runs\nunit-tests and filters the output into stdout and stderr. But maybe it's\ntoo ugly. I think --verbose-log works because we know in the test code\nwhen we are outputting TAP on stdout, and everything else goes to the\nlog. But because it's all generated by the unit-tests bin, we'd end up\nhaving to parse its output ourselves and redirect some to stdout and\nsome to the log. It might be less work to implement --verbose-log in\nour clar harness.\n\nAnyway. Those are all of my complaints. For now. ;) I don't know if I'll\nwork on any of them or not, and maybe people more familiar with clar can\noffer suggestions. But I thought it worth documenting the experience.\n\n-Peff\n"},{"id":"531518","messageId":"bd0a8a76-fccb-4b6c-abb7-b53dd890e9e0@gmail.com","threadId":"64468","inReplyTo":"20251130134625.GA199421@coredump.intra.peff.net","subject":"Re: my complaints with clar","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-12-01T14:16:13Z","receivedAt":"2025-12-01T14:16:17Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 30/11/2025 13:46, Jeff King wrote:\n> \n> cl_invoke(check_int_full(\"0\", 0));\n> cl_invoke(check_int_full(\"11\", 11));\n> cl_invoke(check_int_full(\"-23\", -23));\n> cl_invoke(check_int_full(\"+23\", 23));\n> \n> gives you the line number in the caller. Better, but there's a lot of\n> cross-referencing the line numbers (plus sprinkling cl_invoke everywhere\n> is ugly).\n\nThe README for the old unit-testing framework recommended wrapping \nhelper functions in a macro to pass the file and line numbers from the \ncalling site. Perhaps we should do the same with clar\n\n#define check_int_full(input, expect) cl_invoke(check_int_full(input, \nexpect))\n\n> What I really would have liked is some notion of \"context\". If the\n> helper could have done:\n> \n> cl_context(\"input: %.*s\", (int)len, buf);\n> \n> or similar, and failed assertions print that context, then that would\n> have made the failing part of the test easy to see, even without using\n> cl_invoke() at all.\n\nIf you're writing a helper function you might want to use cl_failf() \ninstead of cl_assert_* to provide more context but it's a pain that you \ncan't just use the builtin assertions. I've not used them but there are \nassertions named cl_assert_*_ which I think let you add some context. \nOne of the features of the conversion of our unit tests from the old \nframework to clar has been a degradation of the diagnostic messages when \na test fails.\n\n>> +void test_parse_int__basic(void)\n>> +{\n>> +\tcl_invoke(check_int_full(\"0\", 0));\n>> +\tcl_invoke(check_int_full(\"11\", 11));\n>> +\tcl_invoke(check_int_full(\"-23\", -23));\n>> +\tcl_invoke(check_int_full(\"+23\", 23));\n>> +\tcl_invoke(check_int_str(\" 31337 \", 7, 0, 31337));\n>> +\n>> +\tcl_invoke(check_int_err(\" garbage\", EINVAL));\n>> +\tcl_invoke(check_int_err(\"\", EINVAL));\n>> +\tcl_invoke(check_int_err(\"-\", EINVAL));\n>> +\n>> +\tcl_invoke(check_int(\"123\", 2, 2, 0, 12));\n>> +}\n> \n> into a series of nine separate tests, each of which gets a name. But\n> each of those tests is at least five lines of boilerplate, which sucks\n> (plus you have to come up with syntactically valid C names for them).\n\nYes that's a pain. One of the nice things about the old framework was \nthe the TEST() macro just took an expression and created a test case out \nof it which worked well for tests like this and meant you could have \ntable driven tests where each entry in the table was a separate test case.\n\n>> +\t/*\n>> +\t * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro\n>> +\t * casts to int under the hood, corrupting the values.\n>> +\t */\n>> +\tclar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,\n>> +\t\t\t CLAR_CURRENT_LINE,\n>> +\t\t\t \"expect_result != result\", 1,\n>> +\t\t\t \"%\"PRIuMAX, expect_result, result);\n>> +}\n> \n> This was an exciting bug to track down. If you use i_fmt() here, you get\n> some neat undefined behavior. It worked for gcc, but failed with clang\n> (but only with -O2!).\n> \n> Obviously this was me using it wrong, and the \"i\" in the macro should\n> have been a hint. But this invocation is kind of ugly, with the explicit\n> mentions of internal CLAR variables. clar__assert_equal() understands\n> PRIuMAX as a comparator, but there doesn't appear to be any macro to use\n> it nicely.\n> \n> Should there be a generic cl_assert_equal() that fills in the first\n> few parameters but is otherwise type-agnostic?\n\nPatrick's got a PR open for that at \nhttps://github.com/clar-test/clar/pull/117 it seems to have got stuck \nbecause of a lack of review.\n\n> # start of suite 10: parse_int\n> not ok 59 - parse_int::basic\n> ---\n> reason: |\n> expect_result != result\n> 10 != 11\n> at:\n> file: 't/unit-tests/u-parse-int.c'\n> line: 41\n> function: 'test_parse_int__basic'\n> ---\n> \n> OK, but \"prove t/unit-tests/bin/unit-tests\" gives me:\n> \n> t/unit-tests/bin/unit-tests .. Failed 1/59 subtests\n> \n> Test Summary Report\n> -------------------\n> t/unit-tests/bin/unit-tests (Wstat: (none) Tests: 59 Failed: 1)\n> Failed test: 59\n> Parse errors: Badly formed hash line: '---' at /usr/share/perl/5.40/TAP/Parser/YAMLish/Reader.pm line 244.\n> \n> Yuck. It actually does have what I need (that test 59 was the failure),\n> so the extra parse error is mostly a red herring (though it does prevent\n> us finding any further failures). I think in TAP that arbitrary output\n> is supposed to be prefixed with a \"#\".\n\nTAP also allows you to embed YAML and unfortunately that's what clar \ntries to do but that last \" ---\" line should be \" ...\". With the \ndiff below (which I'm afraid thunderbird will probably mangle) prove \nparses the output correctly but still does not print the error message. \nI'll update clar's self tests and open a PR later this week.\n\n---- 8< ----\ndiff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h\nindex 89b66591d75..6a2321b399d 100644\n--- a/t/unit-tests/clar/clar/print.h\n+++ b/t/unit-tests/clar/clar/print.h\n@@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char \n*suite_name, const char *test_name,\n printf(\" file: '\"); \nprint_escaped(error->file); printf(\"'\\n\");\n printf(\" line: %\" PRIuMAX \"\\n\", \nerror->line_number);\n printf(\" function: '%s'\\n\", error->function);\n- printf(\" ---\\n\");\n+ printf(\" ...\\n\");\n }\n\n break;\n---- >8 ----\n\n> In test-lib.sh, we solve this by\n> only allowing \"--verbose-log\", not regular \"-v\", under a TAP harness.\n> \n> I kind of wonder if we should have t0011-unit-tests.sh that simply runs\n> unit-tests and filters the output into stdout and stderr.\n\nI don't have a strong opinion on this but now that we don't run the unit \ntests in parallel because clar links them all into a single executable \nthere is less reason to use prove.\n\n\nThanks\n\nPhillip\n\n"},{"id":"531653","messageId":"aTFsA-jJqcRZJs53@pks.im","threadId":"64468","inReplyTo":"bd0a8a76-fccb-4b6c-abb7-b53dd890e9e0@gmail.com","subject":"Re: my complaints with clar","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-04T11:09:55Z","receivedAt":"2025-12-04T11:10:02Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 01, 2025 at 02:16:13PM +0000, Phillip Wood wrote:\n> On 30/11/2025 13:46, Jeff King wrote:\n> > > +\t/*\n> > > +\t * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro\n> > > +\t * casts to int under the hood, corrupting the values.\n> > > +\t */\n> > > +\tclar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,\n> > > +\t\t\t CLAR_CURRENT_LINE,\n> > > +\t\t\t \"expect_result != result\", 1,\n> > > +\t\t\t \"%\"PRIuMAX, expect_result, result);\n> > > +}\n> > \n> > This was an exciting bug to track down. If you use i_fmt() here, you get\n> > some neat undefined behavior. It worked for gcc, but failed with clang\n> > (but only with -O2!).\n> > \n> > Obviously this was me using it wrong, and the \"i\" in the macro should\n> > have been a hint. But this invocation is kind of ugly, with the explicit\n> > mentions of internal CLAR variables. clar__assert_equal() understands\n> > PRIuMAX as a comparator, but there doesn't appear to be any macro to use\n> > it nicely.\n> > \n> > Should there be a generic cl_assert_equal() that fills in the first\n> > few parameters but is otherwise type-agnostic?\n> \n> Patrick's got a PR open for that at\n> https://github.com/clar-test/clar/pull/117 it seems to have got stuck\n> because of a lack of review.\n\nYeah, this is indeed a long-standing issue. As Phillip mentioned I've\nalready had the fix pending, but I lost track and just never merged it.\nPhillip now left a review, and I've polished the PR a bit. I'll wait a\nfew more days before merging it, and then these issues will be a thing\nof the past :)\n\n> > # start of suite 10: parse_int\n> > not ok 59 - parse_int::basic\n> > ---\n> > reason: |\n> > expect_result != result\n> > 10 != 11\n> > at:\n> > file: 't/unit-tests/u-parse-int.c'\n> > line: 41\n> > function: 'test_parse_int__basic'\n> > ---\n> > \n> > OK, but \"prove t/unit-tests/bin/unit-tests\" gives me:\n> > \n> > t/unit-tests/bin/unit-tests .. Failed 1/59 subtests\n> > Test Summary Report\n> > -------------------\n> > t/unit-tests/bin/unit-tests (Wstat: (none) Tests: 59 Failed: 1)\n> > Failed test: 59\n> > Parse errors: Badly formed hash line: '---' at /usr/share/perl/5.40/TAP/Parser/YAMLish/Reader.pm line 244.\n> > \n> > Yuck. It actually does have what I need (that test 59 was the failure),\n> > so the extra parse error is mostly a red herring (though it does prevent\n> > us finding any further failures). I think in TAP that arbitrary output\n> > is supposed to be prefixed with a \"#\".\n> \n> TAP also allows you to embed YAML and unfortunately that's what clar tries\n> to do but that last \" ---\" line should be \" ...\". With the diff below\n> (which I'm afraid thunderbird will probably mangle) prove parses the output\n> correctly but still does not print the error message. I'll update clar's\n> self tests and open a PR later this week.\n> \n> ---- 8< ----\n> diff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h\n> index 89b66591d75..6a2321b399d 100644\n> --- a/t/unit-tests/clar/clar/print.h\n> +++ b/t/unit-tests/clar/clar/print.h\n> @@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char\n> *suite_name, const char *test_name,\n> printf(\" file: '\");\n> print_escaped(error->file); printf(\"'\\n\");\n> printf(\" line: %\" PRIuMAX \"\\n\",\n> error->line_number);\n> printf(\" function: '%s'\\n\", error->function);\n> - printf(\" ---\\n\");\n> + printf(\" ...\\n\");\n> }\n> \n> break;\n> ---- >8 ----\n\nIndeed, this was a plain bug. I've merged your upstream PR, thanks!\n\nI'll send a pull request soonish to update our own version of clar.\n\nThanks for the feedback! Hope that the pending changes will improve the\nstatus quo.\n\nPatrick\n"},{"id":"531654","messageId":"aTFvKOHlm4zfT9dU@pks.im","threadId":"64468","inReplyTo":"20251130131537.GB199335@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-04T11:23:20Z","receivedAt":"2025-12-04T11:23:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Nov 30, 2025 at 08:15:37AM -0500, Jeff King wrote:\n[snip]\n> For the interface:\n> \n> - What do we call it? We have git_parse_int() and friends, which aim\n> to make parsing less error-prone. And in some ways, these are just\n> buffer (rather than string) versions of those functions. But not\n> entirely. Those functions are aimed at parsing a single user-facing\n> value. So they accept a unit prefix (e.g., \"10k\"), which we won't\n> always want. And they insist that the whole string is consumed\n> (rather than passing back an \"end\" pointer).\n> \n> We also have strtol_i() and strtoul_ui() wrappers, which try to make\n> error handling simpler (especially around overflow), but mostly\n> behave like their libc counterparts. These also don't pass out an\n> end pointer, though.\n> \n> So I started a new namespace, \"parse__from_buf\".\n\nI think it would be nice if we could eventually converge towards a\ncommon namespace here. E.g. `strotol_i()` would then become\n`parse_()`, without the `_from_buf()` suffix. That would make it a\nbit more discoverable.\n\nSimilarly, `git_parse_int()` could become `parse__with_units()`\neventually.\n\nThat certainly doesn't have to be part of this series though.\n\n> - Like those other functions above, we use an out-parameter to store\n> the result, which lets us return an error code directly. This avoids\n> the complicated errno dance for detecting overflow that you get with\n> strtol().\n> \n> What should the error code look like? git_parse_int() uses a bool\n> for success/failure. But strtol_ui() uses the syscall-like \"0 is\n> success, -1 is error\" convention.\n> \n> I went with the bool approach here. Since the names are closest to\n> those functions, I thought it would cause the least confusion.\n\nI think that's a sensible choice.\n\n> diff --git a/Makefile b/Makefile\n> index 237b56fc9d..751bd40a9f 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1510,6 +1510,7 @@ CLAR_TEST_SUITES += u-mem-pool\n> CLAR_TEST_SUITES += u-oid-array\n> CLAR_TEST_SUITES += u-oidmap\n> CLAR_TEST_SUITES += u-oidtree\n> +CLAR_TEST_SUITES += u-parse-int\n> CLAR_TEST_SUITES += u-prio-queue\n> CLAR_TEST_SUITES += u-reftable-basics\n> CLAR_TEST_SUITES += u-reftable-block\n> diff --git a/parse.c b/parse.c\n> index f626846def..1dcbcf64a1 100644\n> --- a/parse.c\n> +++ b/parse.c\n> @@ -209,3 +209,99 @@ unsigned long git_env_ulong(const char *k, unsigned long val)\n> \t\tdie(_(\"failed to parse %s\"), k);\n> \treturn val;\n> }\n> +\n> +/*\n> + * Helper that handles both signed/unsigned cases. If \"negate\" is NULL,\n> + * negative values are disallowed. If not NULL and the input is negative,\n> + * the value is range-checked but the caller is responsible for actually doing\n> + * the negatiion. You probably don't want to use this! Use one of\n> + * parse_signed_from_buf() or parse_unsigned_from_buf() below.\n> + */\n> +static bool parse_from_buf_internal(const char *buf, size_t len,\n> +\t\t\t\t const char **ep, bool *negate,\n> +\t\t\t\t uintmax_t *ret, uintmax_t max)\n> +{\n> +\tconst char *end = buf + len;\n> +\tuintmax_t val = 0;\n> +\n> +\twhile (buf < end && isspace(*buf))\n> +\t\tbuf++;\n\nHm. Do we really want to retain the behaviour of skipping leading\nspaces? I think it's a rather weird edge case of `strtol()` and friends,\nand if we can avoid it I'd prefer to not replicate this behaviour.\n\n> diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c\n> new file mode 100644\n> index 0000000000..a1601bb16b\n> --- /dev/null\n> +++ b/t/unit-tests/u-parse-int.c\n> @@ -0,0 +1,98 @@\n[snip]\n> +void test_parse_int__basic(void)\n> +{\n> +\tcl_invoke(check_int_full(\"0\", 0));\n> +\tcl_invoke(check_int_full(\"11\", 11));\n> +\tcl_invoke(check_int_full(\"-23\", -23));\n> +\tcl_invoke(check_int_full(\"+23\", 23));\n> +\n> +\tcl_invoke(check_int_str(\" 31337 \", 7, 0, 31337));\n> +\n> +\tcl_invoke(check_int_err(\" garbage\", EINVAL));\n> +\tcl_invoke(check_int_err(\"\", EINVAL));\n> +\tcl_invoke(check_int_err(\"-\", EINVAL));\n> +\n> +\tcl_invoke(check_int(\"123\", 2, 2, 0, 12));\n> +}\n\nAs Phillip suggested, it might make sense to wrap these `cl_invoke()`\ncalls into a macro.\n\nPatrick\n"},{"id":"531655","messageId":"aTFvL1GV76o-tUJL@pks.im","threadId":"64468","inReplyTo":"20251130131441.GA199335@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] parse: prefer bool to int for boolean returns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-04T11:23:27Z","receivedAt":"2025-12-04T11:23:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Nov 30, 2025 at 08:14:41AM -0500, Jeff King wrote:\n> All of the integer parsing functions in parse.[ch] return an int that is\n> \"0\" for failure or \"1\" for success. Since most of the other functions in\n> Git use \"0\" for success and \"-1\" for failure, this can be confusing.\n> Let's switch the return types to bool to make it clear that we are using\n> this other convention. Callers should not need to update at all.\n> \n> Signed-off-by: Jeff King \n> ---\n> Obviously not strictly necessary for this series, but I think a good\n> idea regardless of the rest of it.\n\nAgreed, I think this is a sensible change as it helps guide users. I\nknow that I was confused by the API several times already.\n\n> diff --git a/parse.c b/parse.c\n> index 48313571aa..f626846def 100644\n> --- a/parse.c\n> +++ b/parse.c\n> int git_parse_maybe_bool_text(const char *value)\n> diff --git a/parse.h b/parse.h\n> index ea32de9a91..f80cc5b9fd 100644\n> --- a/parse.h\n> +++ b/parse.h\n> @@ -1,13 +1,13 @@\n> #ifndef PARSE_H\n> #define PARSE_H\n> \n> -int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);\n> -int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);\n> -int git_parse_ssize_t(const char *, ssize_t *);\n> -int git_parse_ulong(const char *, unsigned long *);\n> -int git_parse_int(const char *value, int *ret);\n> -int git_parse_int64(const char *value, int64_t *ret);\n> -int git_parse_double(const char *value, double *ret);\n> +bool git_parse_signed(const char *value, intmax_t *ret, intmax_t max);\n> +bool git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);\n> +bool git_parse_ssize_t(const char *, ssize_t *);\n> +bool git_parse_ulong(const char *, unsigned long *);\n> +bool git_parse_int(const char *value, int *ret);\n> +bool git_parse_int64(const char *value, int64_t *ret);\n> +bool git_parse_double(const char *value, double *ret);\n\nShould we maybe add a comment to these functions while at it to document\ntheir behaviour?\n\nPatrick\n"},{"id":"531713","messageId":"4d83375b-76e2-4420-80dd-6a04d3201532@gmail.com","threadId":"64468","inReplyTo":"20251130131537.GB199335@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-12-05T16:11:15Z","receivedAt":"2025-12-05T16:11:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 30/11/2025 13:15, Jeff King wrote:\n> If you have a buffer that is not NUL-terminated but want to parse an\n> integer, there aren't many good options. If you use strtol() and\n> friends, you risk running off the end of the buffer if there is no\n> non-digit terminating character. And even if you carefully make sure\n> that there is such a character, ASan's strict-string-check mode will\n> still complain.\n> \n> You can copy bytes into a temporary buffer, terminate it, and then call\n> strtol(), but doing so adds some pitfalls (like making sure you soak up\n> whitespace and leading +/- signs, and reporting overflow for overly long\n> input). Or you can hand-parse the digits, but then you need to take some\n> care to handle overflow (and again, whitespace and +/- signs).\n> \n> These things aren't impossible to do right, but it's error-prone to have\n> to do them in every spot that wants to do such parsing. So let's add\n> some functions which can be used across the code base.\n> \n> There are a few choices regarding the interface and the implementation.\n> \n> First, the implementation:\n> \n> - I went with with parsing the digits (rather than buffering and\n> passing to libc functions). It ends up being a similar amount of\n> code because we have to do some parsing either way. And likewise\n> overflow detection depends on the exact type the caller wants, so we\n> either have to do it by hand or write a separate wrapper for\n> strtol(), strtoumax(), and so on.\n> \n> - Unsigned overflow detection is done using the same techniques as in\n> unsigned_add_overflows(), etc. We can't use those macros directly\n> because our core function is type-agnostic (so the caller passes in\n> the max value, rather than us deriving it on the fly). This is\n> similar to how git_parse_int(), etc, work.\n> \n> - Signed overflow detection assumes that we can express a negative\n> value with magnitude one larger than our maximum positive value\n> (e.g., -128..127 for a signed 8-bit value). I doubt this is\n> guaranteed by the standard, but it should hold in practice, and we\n> make the same assumption in git_parse_int(), etc. The nice thing\n> about this is that we can derive the range from the number of bits\n> in the type. For ints, you obviously could use INT_MIN..INT_MAX, but\n> for an arbitrary type, we can use maximum_signed_value_of_type().\n> \n> - I didn't bother with handling bases other than 10. It would\n> complicate the code, and I suspect it won't be needed. We could\n> probably retro-fit it later without too much work, if need be.\n\nThis all sounds sensible to me and an does the interface description.\n\n> +bool parse_unsigned_from_buf(const char *buf, size_t len, const char **ep,\n> +\t\t\t uintmax_t *ret, uintmax_t max)\n> +{\n> +\treturn parse_from_buf_internal(buf, len, ep, NULL, ret, max);\n> +}\n> +\n> +bool parse_signed_from_buf(const char *buf, size_t len, const char **ep,\n> +\t\t\t intmax_t *ret, intmax_t max)\n> +{\n> +\tuintmax_t u_ret;\n> +\tbool negate;\n> +\n> +\tif (!parse_from_buf_internal(buf, len, ep, &negate, &u_ret, max))\n> +\t\treturn false;\n> +\t/*\n> +\t * Range already checked internally, but we must apply negation\n> +\t * ourselves since only we have the signed integer type.\n> +\t */\n> +\tif (negate) {\n> +\t\t*ret = u_ret;\n> +\t\t*ret = -*ret;\n\nIf we're parsing INTMAX_MIN then this negation tries to calculate \n-INTMAX_MIN which is undefined (I've added some tests for parsing \nINTMAX_MAX and INTMAX_MIN at [1] and verified that UBSAN is triggered \nwhen parsing INTMAX_MIN). We could do\n\n\t\t*ret = u_ret;\n\t\tif (*ret != INTMAX_MIN)\n\t\t\t*ret = -*ret;\n\nbut I think it might be easier to alter parse_from_buf_internal() to \nmake \"negate\" a local variable, change the function argument to \"bool \nallow_negative\" and do\n\n\t\t*ret = negate ? 0u - val : val;\n\nThen parse_signed_from_buf() can do \"*ret = *u_ret;\" to convert the \noutput of parse_from_buf_internal() to a signed value.\n\n> diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c\n> new file mode 100644\n> index 0000000000..a1601bb16b\n> --- /dev/null\n> +++ b/t/unit-tests/u-parse-int.c\n> @@ -0,0 +1,98 @@\n> +#include \"unit-test.h\"\n> +#include \"parse.h\"\n> +\n> +static void check_int(const char *buf, size_t len,\n> +\t\t size_t expect_ep_ofs, int expect_errno,\n> +\t\t int expect_result)\n> +{\n> +\tconst char *ep;\n> +\tint result;\n\nDo we want to set errno=0 here so that we can be sure it has been set by \nparse_int_from_buf() when we check it below?\n\nThanks\n\nPhillip\n\n[1] \nhttps://github.com/phillipwood/git/commit/e061e3e640db01d4fcf54d265d33352235151973\n\n> +\tbool ok = parse_int_from_buf(buf, len, &ep, &result);\n> +\n> +\tif (expect_errno) {\n> +\t\tcl_assert(!ok);\n> +\t\tcl_assert_equal_i(expect_errno, errno);\n> +\t\treturn;\n> +\t}\n> +\n> +\tcl_assert(ok);\n> +\tcl_assert_equal_i(expect_result, result);\n> +\tcl_assert_equal_i(expect_ep_ofs, ep - buf);\n> +}\n> +\n> +static void check_int_str(const char *buf, size_t ofs, int err, int res)\n> +{\n> +\tcheck_int(buf, strlen(buf), ofs, err, res);\n> +}\n> +\n> +static void check_int_full(const char *buf, int res)\n> +{\n> +\tcheck_int_str(buf, strlen(buf), 0, res);\n> +}\n> +\n> +static void check_int_err(const char *buf, int err)\n> +{\n> +\tcheck_int(buf, strlen(buf), 0, err, 0);\n> +}\n> +\n> +void test_parse_int__basic(void)\n> +{\n> +\tcl_invoke(check_int_full(\"0\", 0));\n> +\tcl_invoke(check_int_full(\"11\", 11));\n> +\tcl_invoke(check_int_full(\"-23\", -23));\n> +\tcl_invoke(check_int_full(\"+23\", 23));\n> +\n> +\tcl_invoke(check_int_str(\" 31337 \", 7, 0, 31337));\n> +\n> +\tcl_invoke(check_int_err(\" garbage\", EINVAL));\n> +\tcl_invoke(check_int_err(\"\", EINVAL));\n> +\tcl_invoke(check_int_err(\"-\", EINVAL));\n> +\n> +\tcl_invoke(check_int(\"123\", 2, 2, 0, 12));\n> +}\n> +\n> +void test_parse_int__range(void)\n> +{\n> +\t/*\n> +\t * These assume a 32-bit int. We could avoid that with some\n> +\t * conditionals, but it's probably better for the test to\n> +\t * fail noisily and we can decide how to handle it then.\n> +\t */\n> +\tcl_invoke(check_int_full(\"2147483647\", 2147483647));\n> +\tcl_invoke(check_int_err(\"2147483648\", ERANGE));\n> +\tcl_invoke(check_int_full(\"-2147483647\", -2147483647));\n> +\tcl_invoke(check_int_full(\"-2147483648\", -2147483648));\n> +\tcl_invoke(check_int_err(\"-2147483649\", ERANGE));\n> +}\n> +\n> +static void check_unsigned(const char *buf, uintmax_t max,\n> +\t\t\t int expect_errno, uintmax_t expect_result)\n> +{\n> +\tconst char *ep;\n> +\tuintmax_t result;\n> +\tbool ok = parse_unsigned_from_buf(buf, strlen(buf), &ep, &result, max);\n> +\n> +\tif (expect_errno) {\n> +\t\tcl_assert(!ok);\n> +\t\tcl_assert_equal_i(expect_errno, errno);\n> +\t\treturn;\n> +\t}\n> +\n> +\tcl_assert(ok);\n> +\tcl_assert_equal_s(ep, \"\");\n> +\t/*\n> +\t * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro\n> +\t * casts to int under the hood, corrupting the values.\n> +\t */\n> +\tclar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,\n> +\t\t\t CLAR_CURRENT_LINE,\n> +\t\t\t \"expect_result != result\", 1,\n> +\t\t\t \"%\"PRIuMAX, expect_result, result);\n> +}\n> +\n> +void test_parse_int__unsigned(void)\n> +{\n> +\tcl_invoke(check_unsigned(\"4294967295\", UINT_MAX, 0, 4294967295U));\n> +\tcl_invoke(check_unsigned(\"1053\", 1000, ERANGE, 0));\n> +\tcl_invoke(check_unsigned(\"-17\", UINT_MAX, EINVAL, 0));\n> +}\n\n"},{"id":"531722","messageId":"20251205183057.GA33447@coredump.intra.peff.net","threadId":"64468","inReplyTo":"aTFsA-jJqcRZJs53@pks.im","subject":"Re: my complaints with clar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-12-05T18:30:57Z","receivedAt":"2025-12-05T18:30:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 04, 2025 at 12:09:55PM +0100, Patrick Steinhardt wrote:\n\n> > Patrick's got a PR open for that at\n> > https://github.com/clar-test/clar/pull/117 it seems to have got stuck\n> > because of a lack of review.\n> \n> Yeah, this is indeed a long-standing issue. As Phillip mentioned I've\n> already had the fix pending, but I lost track and just never merged it.\n> Phillip now left a review, and I've polished the PR a bit. I'll wait a\n> few more days before merging it, and then these issues will be a thing\n> of the past :)\n\nGreat, thanks.\n\n> > ---- 8< ----\n> > diff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h\n> > index 89b66591d75..6a2321b399d 100644\n> > --- a/t/unit-tests/clar/clar/print.h\n> > +++ b/t/unit-tests/clar/clar/print.h\n> > @@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char\n> > *suite_name, const char *test_name,\n> > printf(\" file: '\");\n> > print_escaped(error->file); printf(\"'\\n\");\n> > printf(\" line: %\" PRIuMAX \"\\n\",\n> > error->line_number);\n> > printf(\" function: '%s'\\n\", error->function);\n> > - printf(\" ---\\n\");\n> > + printf(\" ...\\n\");\n> > }\n> > \n> > break;\n> > ---- >8 ----\n> \n> Indeed, this was a plain bug. I've merged your upstream PR, thanks!\n> \n> I'll send a pull request soonish to update our own version of clar.\n> \n> Thanks for the feedback! Hope that the pending changes will improve the\n> status quo.\n\nIt is certainly better for the TAP parser not to choke on the output,\nbut the more fundamental issue remains that the output is never stored\nor relayed anywhere in our CI runs.\n\nI looked at implementing --verbose-log in our unit-tests clar wrapper,\nbut it's tricky. For one thing, we have to know where to store the\ntest-results (so understanding $TEST_OUTPUT_DIRECTORY and how it may be\nset). Plus, we would then need to duplicate much of the output, going to\nboth the log and to stdout. And all of the output routines are inside\nclar. So we'd need hooks there.\n\nThe regular test suite uses \"tee\" to duplicate the output without the\nscript having to worry about it. We could use the same trick here.\n\nAnd an easy way to solve both issues is to just call it from the test\nsuite, letting it handle --verbose-log itself!\n\nSomething like this seems to work:\n\n #!/bin/sh\n test_description='run clar unit tests'\n . ./test-lib.sh\n exec \"$TEST_DIRECTORY/unit-tests/bin/unit-tests\" ${immediate:+-i}\n\nWe have to \"exec\" there so that we skip out on the exit handler that\nchecks we called test_done. And we obviously cannot call that, because\nit outputs extra TAP.\n\nBut the exec means we also miss some cleanup, like removing our trash\ndirectory.\n\nProbably we'd want a mode in the shell test suite that says \"this thing\nis going to generate TAP, just stay out of its way\". We used to have\ntest_external, but it was removed in 5beca49a0b (test-lib: simplify by\nremoving test_external, 2022-07-28). The \"modern\" alternative is:\n\n test_expect_success 'run unit-tests' '\n\tunit-tests/bin/unit-tests\n '\n\nwhich feels worse (the outer layer of TAP output is all-or-nothing, and\nthe fact that the unit tests produce TAP is irrelevant). I think it\nroughly accomplishes the same thing, but it might be worth looking again\nat the reasons for dropping test_external in the first place.\n\n-Peff\n"},{"id":"534292","messageId":"xmqqldhsxawm.fsf@gitster.g","threadId":"64468","inReplyTo":"4d83375b-76e2-4420-80dd-6a04d3201532@gmail.com","subject":"Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T20:54:33Z","receivedAt":"2026-01-20T20:54:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> There are a few choices regarding the interface and the implementation.\n>> \n>> First, the implementation:\n>> ...\n> This all sounds sensible to me and an does the interface description.\n> ...\n> If we're parsing INTMAX_MIN then this negation tries to calculate \n> -INTMAX_MIN which is undefined (I've added some tests for parsing \n> INTMAX_MAX and INTMAX_MIN at [1] and verified that UBSAN is triggered \n> when parsing INTMAX_MIN). We could do\n>\n> \t\t*ret = u_ret;\n> \t\tif (*ret != INTMAX_MIN)\n> \t\t\t*ret = -*ret;\n>\n> but I think it might be easier to alter parse_from_buf_internal() to \n> make \"negate\" a local variable, change the function argument to \"bool \n> allow_negative\" and do\n>\n> \t\t*ret = negate ? 0u - val : val;\n>\n> Then parse_signed_from_buf() can do \"*ret = *u_ret;\" to convert the \n> output of parse_from_buf_internal() to a signed value.\n>\n>> diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c\n>> new file mode 100644\n>> index 0000000000..a1601bb16b\n>> --- /dev/null\n>> +++ b/t/unit-tests/u-parse-int.c\n>> @@ -0,0 +1,98 @@\n>> +#include \"unit-test.h\"\n>> +#include \"parse.h\"\n>> +\n>> +static void check_int(const char *buf, size_t len,\n>> +\t\t size_t expect_ep_ofs, int expect_errno,\n>> +\t\t int expect_result)\n>> +{\n>> +\tconst char *ep;\n>> +\tint result;\n>\n> Do we want to set errno=0 here so that we can be sure it has been set by \n> parse_int_from_buf() when we check it below?\n\n\nAfter this message, the discussion stopped and the topic has been\ndormant since then for a month and a half. I'd drop the topic from\n'seen' soonish but that does not mean an improved version of this\npatch is unwelcome.\n\nThanks.\n"},{"id":"534309","messageId":"20260121052750.GB567009@coredump.intra.peff.net","threadId":"64468","inReplyTo":"xmqqldhsxawm.fsf@gitster.g","subject":"Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-21T05:27:50Z","receivedAt":"2026-01-21T05:27:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 20, 2026 at 12:54:33PM -0800, Junio C Hamano wrote:\n\n> After this message, the discussion stopped and the topic has been\n> dormant since then for a month and a half. I'd drop the topic from\n> 'seen' soonish but that does not mean an improved version of this\n> patch is unwelcome.\n\nYeah, it's been low on my todo list since then. I think it is fine to\ndrop it for now, and I'll eventually get around to re-rolling.\n\n-Peff\n"}]}