On Sun, Aug 25, 2019 at 01:13:48PM +0900, Mike Hommey wrote:
> 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.
I think this is stronger than just "most of the time". It's an invariant
for strbufs to have a NUL, so the only case where detaching isn't a noop
is the empty slopbuf case you mention.
Splitting hairs, perhaps, but I think with that explanation, we could
probably argue that this case will never come up: strbuf_getline will
either have allocated a buffer or will have returned EOF.
That said, I do think it's quite confusing and is worth fixing, both for
readability and for future-proofing. But...
> 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 whole thing is a sign that the code is Doing It Wrong. Whoever is
taking ownership of the buffer should be calling strbuf_detach() at that
point.
It's a bit tricky, though, because we take ownership and then expect
people still look at command_buf.buf. Which makes me concerned that
there are other operations which might modify the buffer and have the
same issue.
I think it would be much easier to follow if we simply used the same
command_buf over and over, and just copied into the history. The cost is
about the same (we still alloc once per line, though we do an extra
memcpy now). So I thought something like this would work (we don't need
to convert those detaches into a strbuf_reset() calls because
strbuf_getline does so automatically):
diff --git a/fast-import.c b/fast-import.c
index b44d6a467e..31207acd61 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1763,7 +1763,6 @@ static int read_next_command(void)
} else {
struct recent_command *rc;
- strbuf_detach(&command_buf, NULL);
stdin_eof = strbuf_getline_lf(&command_buf, stdin);
if (stdin_eof)
return EOF;
@@ -1784,7 +1783,7 @@ static int read_next_command(void)
free(rc->buf);
}
- rc->buf = command_buf.buf;
+ rc->buf = xstrdup(command_buf.buf);
rc->prev = cmd_tail;
rc->next = cmd_hist.prev;
rc->prev->next = rc;
@@ -1833,7 +1832,6 @@ 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);
for (;;) {
if (strbuf_getline_lf(&command_buf, stdin) == EOF)
die("EOF in data (terminator '%s' not found)", term);
But it doesn't! It turns out that there are other places in the code
which assume they can call read_next_command() while hanging onto the
existing buffer. Which only works in the old code because the history
feature happens to hold on to the old pointer.
If I do this on top, then all tests pass:
diff --git a/fast-import.c b/fast-import.c
index 31207acd61..1f9160b645 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2586,7 +2586,7 @@ static void parse_new_commit(const char *arg)
struct branch *b;
char *author = NULL;
char *committer = NULL;
- const char *encoding = NULL;
+ char *encoding = NULL;
struct hash_list *merge_list = NULL;
unsigned int merge_count;
unsigned char prev_fanout, new_fanout;
@@ -2609,8 +2609,10 @@ static void parse_new_commit(const char *arg)
}
if (!committer)
die("Expected committer but didn't get one");
- if (skip_prefix(command_buf.buf, "encoding ", &encoding))
+ if (skip_prefix(command_buf.buf, "encoding ", &v)) {
+ encoding = xstrdup(v);
read_next_command();
+ }
parse_data(&msg, 0, NULL);
read_next_command();
parse_from(b);
@@ -2684,6 +2686,7 @@ static void parse_new_commit(const char *arg)
strbuf_addbuf(&new_data, &msg);
free(author);
free(committer);
+ free(encoding);
if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))
b->pack_id = pack_id;
And I think this is actually a real bug in the current code! We keep a
pointer to the encoding string, which survives because of the history.
But that history is bounded, and we could have an indefinite number of
changed files in the middle. If I modify t9300 like this:
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 141b7fa35e..d4bbe630d5 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3314,6 +3314,11 @@ test_expect_success 'X: handling encoding' '
printf "Pi: \360\nCOMMIT\n" >>input &&
+ for i in $(test_seq 200)
+ do
+ echo "M 644 $EMPTY_BLOB file-$i"
+ done >>input &&
+
git fast-import <input &&
git cat-file -p encoding | grep $(printf "\360") &&
git log -1 --format=%B encoding | grep $(printf "\317\200")
and run the test suite (using an unmodified git, without the earlier
patches I showed) then the test fails, putting garbage into the encoding
header (and when compiled with ASan, reports a use-after-free).
So I think the way the string handling is currently done papers over
such problems. You only see the problem if you have a hundred or more
modified files, so it works _most_ of the time.
That implies to me it's worth following a fix like the one I showed
above. I am slightly concerned there are other similar cases to the
"encoding" one lurking (and they _might_ not be bugs already, if there's
a fixed number of reads between the pointer being saved and being used),
but it seems worth the risk.
-Peff
