Re: [PATCH 1/5] check-ignore: move setup into cmd_check_ignore()

From: Adam Spiers <git@adamspiers.org>
Date: Apr 11, 2013, 11:05 UTC
Message-ID: <20130411110511.GB24296@pacific.linksys.moosehall>
In-Reply-To: <20130411052553.GA28915@sigill.intra.peff.net>

On Thu, Apr 11, 2013 at 01:25:53AM -0400, Jeff King wrote:

Show 19 quoted lines

> On Thu, Apr 11, 2013 at 02:59:31AM +0100, Adam Spiers wrote:
> > -static int check_ignore(const char *prefix, const char **pathspec)
> > +static int check_ignore(struct path_exclude_check check,
> > +			const char *prefix, const char **pathspec)
> 
> Did you mean to pass the struct by value here? If it is truly a per-path
> value, shouldn't it be declared and initialized inside here? Otherwise
> you risk one invocation munging things that the struct points to, but
> the caller's copy does not know about the change.
> 
> In particular, I see that the struct includes a strbuf. What happens
> when one invocation of check_ignore grows the strbuf, then returns? The
> copy of the struct in the caller will not know that the buffer it is
> pointing to is now bogus.
> 
> > -static int check_ignore_stdin_paths(const char *prefix)
> > +static int check_ignore_stdin_paths(struct path_exclude_check check, const char *prefix)
> 
> Ditto here.

It's not a per-path value; it's supposed to be reused across checks for multiple paths, as explained in the comments above last_exclude_matching_path():

    ...
     * A path to a directory known to be excluded is left in check->path to
     * optimize for repeated checks for files in the same excluded directory.
     */
    struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
    ...

So I think you're probably right that there is potential for check->path to become effectively corrupted due to the caller not seeing the reallocation. I'll change this too.