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.
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.
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);
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);
for (;;) {
if (strbuf_getline_lf(&command_buf, stdin) == EOF)
die("EOF in data (terminator '%s' not found)", term);
--
2.23.0
