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@xxxxxxxx> 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
