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
