list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Johannes Schindelin via GitGitGadget <>
Cc:, Junio C Hamano <>,
	Johannes Schindelin <>
Subject: Re: [PATCH 1/1] untracked cache: fix off-by-one
Date: Wed, 10 Apr 2019 12:20:29 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, Apr 10, 2019 at 05:56:48AM -0700, Johannes Schindelin via GitGitGadget wrote:

> Probably in the endeavor to avoid the `calloc()` implied by
> `FLEX_ALLOC_STR()` (it is hard to know why exactly, the commit message
> of that commit is a bit parsimonious with information), it calls
> `malloc()` manually and then `memcpy()`s the bits and pieces into place.

You have a talent for understatement. :)

> It allocates the size of `struct untracked_cache_dir` plus the string
> length of the untracked file name, then copies the information in two
> steps: first the fixed-size metadata, then the name. And here lies the
> rub: it includes the trailing NUL byte in the name.
> If `FLEX_ARRAY` is defined as 0, this results in a buffer overrun.
> To fix this, let's just add 1, for the trailing NUL byte. Technically,
> this overallocates on platforms where `FLEX_ARRAY` is 1, but it should
> not matter much in reality, as `malloc()` usually overallocates anyway,
> unless the size to allocate aligns exactly with some internal chunk size
> (see below for more on that).

Yeah, every struct on a platform where FLEX_ARRAY is 1 ends up
over-allocated by 1 byte. We could account for that in all of our flex
allocations, but I don't it affects enough platforms to be worth the

> The real strange thing is that neither valgrind nor DrMemory catches
> this bug. In this developer's tests, a `memcpy()` (but not a
> `memset()`!) could write up to 4 bytes after the allocated memory range
> before valgrind would start reporting an issue.

I couldn't get it to trigger with ASan, either.  I assume it's due to
alignment (i.e., those tools are stuck poisoning at the end of a page,
but the start of the struct needs to be page-aligned). But that would
imply that we could trigger it with different path lengths, which I
wasn't able to do. So I dunno.

> However, when running Git built with nedmalloc as allocator, under rare
> conditions (and inconsistently at that), this bug triggered an `abort()`
> because nedmalloc rounds up the size to be `malloc()`ed to a multiple of
> a certain chunk size, then adds a few bytes to be used for storing some
> internal state. If there is no rounding up to do (because the size is
> already a multiple of that chunk size), and if the buffer is overrun as
> in the code patched in this commit, the internal state is corrupted.
> The scenario that triggered this here bug fix entailed a git.git
> checkout with an extra copy of the source code in an untracked
> subdirectory, meaning that there was an untracked subdirectory called
> "thunderbird-patch-inline" whose name's length is exactly 24 bytes,
> which, added to the size of above-mentioned `struct untracked_cache_dir`
> that weighs in with 104 bytes on a 64-bit system, amounts to 128,
> aligning perfectly with nedmalloc's chunk size.

Right, that makes sense that the length would impact it there.

> As there is no obvious way to trigger this bug reliably, on all
> platforms supported by Git, and as the bug is obvious enough, this patch
> comes without a regression test.

Makes sense. This code path should be well-covered by the existing tests
anyway, so even if we could get those tools to trigger, I don't think
there would be much point in adding a new test.

> diff --git a/dir.c b/dir.c
> index b2cabadf25..f5293a6536 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2760,7 +2760,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
>  	next = data + len + 1;
>  	if (next > rd->end)
>  		return -1;
> -	*untracked_ = untracked = xmalloc(st_add(sizeof(*untracked), len));
> +	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), len, 1));
>  	memcpy(untracked, &ud, sizeof(ud));
>  	memcpy(untracked->name, data, len + 1);

This is obviously correct, even just from the context.

IMHO it is worth it in cases like this to just use FLEX_ALLOC for
simplicity; calloc() is not too expensive. However, there's an
interesting subtlety there. Our length comes from this line just above
your hunk:

  len = strlen((const char *)data);

how do we know that data actually contains a NUL? It's ultimately
pointing to our mmap of the index file. So I think a malformed index
would potentially cause us to go off the end of the array and read
arbitrary memory.

The right thing is probably something like:

  eos = memchr(data, '\0', end - data);
  if (!eos)
	return error("malformed untracked cache extension");
  len = eos - data;

I wouldn't be at all surprised if other bits of the index code have the
same issue, though. And at any rate, thinking about that should
definitely not hold up your fix.


  reply	other threads:[~2019-04-10 16:20 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-10 12:56 [PATCH 0/1] Fix an off-by-one bug in the untracked cache code Johannes Schindelin via GitGitGadget
2019-04-10 12:56 ` [PATCH 1/1] untracked cache: fix off-by-one Johannes Schindelin via GitGitGadget
2019-04-10 16:20   ` Jeff King [this message]
2019-04-12  1:48     ` Junio C Hamano
2019-04-18 21:14       ` [PATCH 0/3] untracked cache parsing fixups Jeff King
2019-04-18 21:17         ` [PATCH 1/3] untracked-cache: be defensive about missing NULs in index Jeff King
2019-04-19  5:29           ` Junio C Hamano
2019-04-18 21:17         ` [PATCH 2/3] untracked-cache: simplify parsing by dropping "next" Jeff King
2019-04-19  5:33           ` Junio C Hamano
2019-04-18 21:18         ` [PATCH 3/3] untracked-cache: simplify parsing by dropping "len" Jeff King
2019-04-18 21:24         ` [PATCH 4/3] untracked-cache: use FLEX_ALLOC to create internal structs Jeff King
2019-04-19  9:18           ` Duy Nguyen
2019-04-19 19:43             ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).