diff options
| author | Jeff King <peff@peff.net> | 2025-11-18 04:12:20 -0500 |
|---|---|---|
| committer | Junio C Hamano <gitster@pobox.com> | 2025-11-18 09:36:11 -0800 |
| commit | 0b6ec075df2ac77a4792b8b1a7290a36b636012b (patch) | |
| tree | 896827dd47d8e3fa57669c21765d443d1ddc7263 | |
| parent | c4c9089584d0ed04978e8d0945b2ba2985e67bd3 (diff) | |
| download | git-0b6ec075df2ac77a4792b8b1a7290a36b636012b.tar.gz | |
fsck: assert newline presence in fsck_ident()
The fsck code purports to handle buffers that are not NUL-terminated,
but fsck_ident() uses some string functions. This works OK in practice,
as explained in 8e4309038f (fsck: do not assume NUL-termination of
buffers, 2023-01-19). Before calling fsck_ident() we'll have called
verify_headers(), which makes sure we have at least a trailing newline.
And none of our string-like functions will walk past that newline.
However, that makes this code at the top of fsck_ident() very confusing:
*ident = strchrnul(*ident, '\n');
if (**ident == '\n')
(*ident)++;
We should always see that newline, or our memory safety assumptions have
been violated! Further, using strchrnul() is weird, since the whole
point is that if the newline is not there, we don't necessarily have a
NUL at all, and might read off the end of the buffer.
So let's have callers pass in the boundary of our buffer, which lets us
safely find the newline with memchr(). And if it is not there, this is a
BUG(), because it means our caller did not validate the input with
verify_headers() as it was supposed to (and we are better off bailing
rather than having memory-safety problems).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
| -rw-r--r-- | fsck.c | 16 |
1 files changed, 9 insertions, 7 deletions
@@ -859,16 +859,18 @@ static int verify_headers(const void *data, unsigned long size, FSCK_MSG_UNTERMINATED_HEADER, "unterminated header"); } -static int fsck_ident(const char **ident, +static int fsck_ident(const char **ident, const char *ident_end, const struct object_id *oid, enum object_type type, struct fsck_options *options) { const char *p = *ident; + const char *nl; char *end; - *ident = strchrnul(*ident, '\n'); - if (**ident == '\n') - (*ident)++; + nl = memchr(p, '\n', ident_end - p); + if (!nl) + BUG("verify_headers() should have made sure we have a newline"); + *ident = nl + 1; if (*p == '<') return report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, "invalid author/committer line - missing space before email"); @@ -957,7 +959,7 @@ static int fsck_commit(const struct object_id *oid, author_count = 0; while (buffer < buffer_end && skip_prefix(buffer, "author ", &buffer)) { author_count++; - err = fsck_ident(&buffer, oid, OBJ_COMMIT, options); + err = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options); if (err) return err; } @@ -969,7 +971,7 @@ static int fsck_commit(const struct object_id *oid, return err; if (buffer >= buffer_end || !skip_prefix(buffer, "committer ", &buffer)) return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, "invalid format - expected 'committer' line"); - err = fsck_ident(&buffer, oid, OBJ_COMMIT, options); + err = fsck_ident(&buffer, buffer_end, oid, OBJ_COMMIT, options); if (err) return err; if (memchr(buffer_begin, '\0', size)) { @@ -1064,7 +1066,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer, goto done; } else - ret = fsck_ident(&buffer, oid, OBJ_TAG, options); + ret = fsck_ident(&buffer, buffer_end, oid, OBJ_TAG, options); if (buffer < buffer_end && !starts_with(buffer, "\n")) { /* |