On Wed, Jan 18, 2023 at 03:35:06PM -0500, Jeff King wrote:

The other option is having the fsck code avoid looking past the size it
was given. I think the intent is that this should work, from commits
like 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the
buffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which
won't respect the size, but I think[1] that's OK because we'll have
parsed up to the end-of-header beforehand (and those functions would
never match past there).

Which would mean that 9a1a3a4d4c (mktag: allow omitting the header/body
\n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of
custom verify_tag(), 2021-01-05) were buggy, and we can just fix them.

That would look something like this:

diff --git a/fsck.c b/fsck.c
index c2c8facd2d..d220276bcb 100644
--- a/fsck.c
+++ b/fsck.c

@@ -898,6 +898,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
 {
 	int ret = 0;
 	char *eol;
+	const char *eob = buffer + size;
 	struct strbuf sb = STRBUF_INIT;
 	const char *p;
 

@@ -960,10 +961,8 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
 	}
 	else
 		ret = fsck_ident(&buffer, oid, OBJ_TAG, options);
-	if (!*buffer)
-		goto done;
 
-	if (!starts_with(buffer, "\n")) {
+	if (buffer != eob && *buffer != '\n') {
 		/*
 		 * The verify_headers() check will allow
 		 * e.g. "[...]tagger <tagger>\nsome

Changing the starts_with() is not strictly necessary, but I think it
makes it more clear that we are only going to look at the one character
we confirmed is still valid inside the buffer.

This is enough to have the whole test suite pass with ASan/UBSan after
my series. But as I said earlier, I'd want to look carefully at the rest
of the fsck code to make sure there aren't any other possible inputs
that could look past the end of the buffer.

-Peff