From: Elijah Newren <newren@gmail.com>
Date: Mon, 26 Aug 2019 18:28:58 GMT
Subject: Re: [PATCH 1/2] fast-import: duplicate parsed encoding string
Message-ID: <CABPp-BG3ScDk6_QcjyouqnHrXO2A1mWPEcPh5FnS1popKFzr-Q@mail.gmail.com>
In-Reply-To: <20190825080821.GA31824@sigill.intra.peff.net>

On Sun, Aug 25, 2019 at 1:08 AM Jeff King <peff@peff.net> wrote:
>
> We read each line of the fast-import stream into the command_buf strbuf.
> When reading a commit, we parse a line like "encoding foo" by storing a
> pointer to "foo", but not making a copy. We may then read an unbounded
> number of other lines (e.g., one for each modified file in the commit),
> each of which writes into command_buf.
>
> This works out in practice for small cases, because we hand off
> ownership of the heap buffer from command_buf to the cmd_hist array, and
> read new commands into a fresh heap buffer. And thus the pointer to
> "foo" remains valid as long as there aren't so many intermediate lines
> that we end up dropping the original "encoding" line from the history.
>
> But as the test modification shows, if we go over our default of 100
> lines, we end up with our encoding string pointing into freed heap
> memory. This seems to fail reliably by writing garbage into the output,
> but running under ASan definitely detects this as a user-after-free.

s/user-after-free/use-after-free/

> We can fix it by duplicating the encoding value, just as we do for other
> parsed lines (e.g., an author line ends up in parse_ident, which copies
> it to a new string).

Eek!  Thanks for fixing this up for me; patch looks good.