On Fri, Sep 17, 2021 at 6:01 PM Jeff King <peff@xxxxxxxx> wrote:
> 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...
> > > ... 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.
Indeed, I got confused by the multiple strbufs with similar names. However...
> However, I do think the code would be much clearer
> if you skipped the strbuf entirely, like:
>
> line_as_string = xmemdupz(line, linelen);
> ...
> buf = line_as_string;
> eol = buf + linelen;
>
> 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).
... as Peff points out, it indeed is unclear why a copy of `line` is
needed at all. It seems like a simple:
buf = line;
eol = buf + linelen;
would more than suffice without making any copies. If you do that, then the:
if (buf != line) {
later in the code -- and which I questioned in my review as making no
sense -- suddenly makes sense.
