Re: [PATCH] fast-import: Reinitialize command_buf rather than detach it.

From: René Scharfe <l.s.r@web.de>
Date: Aug 25, 2019, 12:35 UTC
Message-ID: <94de2333-fc0c-1eb1-6542-32c2971512fc@web.de>
In-Reply-To: <20190825041348.31835-1-mh@glandium.org>

Am 25.08.19 um 06:13 schrieb Mike Hommey:

> command_buf.buf is also stored in cmd_hist, so instead of
> strbuf_release, the current code uses strbuf_detach in order to
> "leak" the buffer as far as the strbuf is concerned.

Hmm.

Show 11 quoted lines

> However, strbuf_detach does more than "leak" the strbuf buffer: it
> possibly reallocates it to ensure a terminating nul character. And when
> that happens, what is already stored in cmd_hist may now be a free()d
> buffer.
>
> In practice, though, command_buf.buf is most of the time a nul
> terminated string, meaning command_buf.len < command_buf.alloc, and
> strbuf_detach is a no-op. BUT, when it's not (e.g. on the first call),
> command_buf.buf is &strbuf_slopbuf. In that case, strbuf_detach does
> allocate a 1 byte buffer to store a nul character in it, which is then
> leaked.

And that's why a pointer to a strbuf buf is valid until the next strbuf_ function call.

Show 21 quoted lines

>
> Since the code using strbuf_detach is assuming it does nothing to
> command_buf.buf, it's overall safer to use strbuf_init, which has
> the same practical effect in the usual case, and works appropriately
> when command_buf is empty.
> ---
>  fast-import.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index b44d6a467e..b1d07efe8c 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1763,7 +1763,9 @@ static int read_next_command(void)
>  		} else {
>  			struct recent_command *rc;
>
> -			strbuf_detach(&command_buf, NULL);
> +			// command_buf is enther empty or also stored in cmd_hist,
> +			// reinitialize it.
> +			strbuf_init(&command_buf, 0);

This is a no-op; strbuf_detach already re-initializes the strbuf.

(And double-slash comments are avoided in Git code..)

((s/enther/either/))

Show 11 quoted lines

>  			stdin_eof = strbuf_getline_lf(&command_buf, stdin);
>  			if (stdin_eof)
>  				return EOF;
> @@ -1833,7 +1835,9 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
>  		char *term = xstrdup(data);
>  		size_t term_len = command_buf.len - (data - command_buf.buf);
>
> -		strbuf_detach(&command_buf, NULL);
> +		// command_buf is enther empty or also stored in cmd_hist,
> +		// reinitialize it.
> +		strbuf_init(&command_buf, 0);

Same here.

>  		for (;;) {
>  			if (strbuf_getline_lf(&command_buf, stdin) == EOF)
>  				die("EOF in data (terminator '%s' not found)", term);
>

strbuf_detach() is handing over ownership of the buffer. Its return value should be stored; grabbing the pointer beforehand is naughty because it can get stale, as you noted. I doubt there is a good reason for ignoring the return value of strbuf_detach(), ever.

René