Re: [PATCH 1/2] fast-import: duplicate parsed encoding string

[Jeff King <peff@peff.net>]

On Mon, Aug 26, 2019 at 11:28:58AM -0700, Elijah Newren wrote:

> 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/

Wow. I self-corrected "user-after-free" at least three other times while
writing this thread. I clearly have a problem. :)

> > 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.

No problem. On the plus side, finding your bug made me think much harder
about the implications of patch 2 (because it's quite subtle and an easy
mistake to make).

-Peff