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