Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

From: Jeff King <peff@peff.net>
Date: Aug 3, 2016, 16:19 UTC
Message-ID: <20160803161911.dxucq7f2pvnoovoc@sigill.intra.peff.net>
In-Reply-To: <20160801214937.4752-2-e@80x24.org>

On Mon, Aug 01, 2016 at 09:49:37PM +0000, Eric Wong wrote:

Show 17 quoted lines

> +static void setup_pager_env(struct argv_array *env)
> +{
> +	const char *pager_env = PAGER_ENV;
> +
> +	while (*pager_env) {
> +		struct strbuf buf = STRBUF_INIT;
> +		const char *cp = strchrnul(pager_env, '=');
> +
> +		if (!*cp)
> +			die("malformed build-time PAGER_ENV");
> +		strbuf_add(&buf, pager_env, cp - pager_env);
> +		cp = strchrnul(pager_env, ' ');
> +		if (!getenv(buf.buf)) {
> +			strbuf_reset(&buf);
> +			strbuf_add(&buf, pager_env, cp - pager_env);
> +			argv_array_push(env, strbuf_detach(&buf, NULL));
> +		}

argv_array handles its own allocation, so this leaks the detached strbuf.

You'd want:

  argv_array_push(env, buf.buf);
  strbuf_release(&buf);

or just:

  argv_array_pushf(env, "%.*s", (int)(cp - pager_env), pager_env);

Also:

> +		strbuf_reset(&buf);

should this be strbuf_release()? If we didn't follow the conditional above (because getenv() told us the variable was already set), then we would not do do the detach/release there, and would finish the loop with memory still allocated by "buf".

-Peff