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.
> 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.
>
> 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/))
> 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é
