Re: [PATCH v5 2/2] pretty: colorize pattern matches in commit messages

[Jeff King <peff@peff.net>]

On Fri, Sep 17, 2021 at 05:14:56PM -0400, Hamza Mahfooz wrote:

> 
> On Fri, Sep 17 2021 at 03:10:12 AM -0400, Eric Sunshine
> <sunshine@sunshineco.com> wrote:
> > `buf` and `eol` seem like an accident waiting to happen...
> > 
> > >  +       line_color = opt->colors[GREP_COLOR_SELECTED];
> > >  +       match_color = opt->colors[GREP_COLOR_MATCH_SELECTED];
> > >  +
> > >  +       while (grep_next_match(opt, buf, eol, ctx, &match, field,
> > > eflags)) {
> > >  +               if (match.rm_so == match.rm_eo)
> > >  +                       break;
> > >  +
> > >  +               strbuf_grow(sb, strlen(line_color) +
> > > strlen(match_color) +
> > >  +                           (2 * strlen(GIT_COLOR_RESET)));
> > 
> > ... because strbuf_grow() may reallocate the underlying buffer, which
> > means that `buf` and `eol` will end up pointing at freed memory, which
> > will be accessed by the next call to grep_next_match().
> 
> I don't see how it's problematic, since `tmp_sb` isn't modified after `buf`
> is initialized (until strbuf_release() is called, of course).

Yes, you are correct. However, I do think the code would be much clearer
if you skipped the strbuf entirely, like:

  char *line_as_string;
  ...
  line_as_string = xmemdupz(line, linelen);
  ...
  buf = line_as_string;
  eol = buf + linelen;
  ...
  free(line_as_string);

which makes it much clearer that you don't intend to modify it further
(especially with all those other calls operating on the _other_ strbuf,
it's hard to see immediately that this is the case).

The "as_string" name is assuming the purpose is to get a NUL-terminated
string. I'm not sure why we need one, though, since we pass the buf/eol
pointers to grep_next_match(). A comment above the xmemdupz() line might
be a good place to explain that (which would help especially if the
reasons change later and we can get rid of it).

-Peff