[1/6] am: use `strbuf_attach()` correctly

Commit Message

Martin Ågren April 18, 2020, 8:18 p.m. UTC

Among other parameters, `strbuf_attach()` takes a length and an amount
of allocated memory. In strbuf.h, it is documented that the latter "must
be larger than the string length, because the string you pass is
supposed to be a NUL-terminated string".

In builtin/am.c, we simply pass in the length of the string twice.

My first assumption was that we'd end up with `alloc == len` and that,
e.g., a subsequent `strbuf_avail(sb)` would evaluate `sb->alloc
- sb->len - 1`, resulting in a huge return value, which could be quite
bad. But luckily, we end up in `strbuf_grow()` where we reallocate a
larger buffer, after which we reinstate a '\0' and everything is fine.

One might ask if the function was documented incorrectly in dd613e6b87
("Strbuf documentation: document most functions", 2008-06-04), but on
the other hand, one really has to wonder whether it's actually useful to
be able to pass in `alloc == len` only to end up performing the
allocation, copying and freeing which this function very much looks like
it would keep us from having to do.

Pass in a value one greater than the length for the `alloc` parameter.
The string has been allocated correctly using the strbuf machinery in
`read_commit_msg()` and we really do have an extra byte at the end with
a NUL. This means both that the buffer is as large as we claim it to be
and that the addition is safe.

Signed-off-by: Martin Ågren <[e-mail]>
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch

diff --git a/builtin/am.c b/builtin/am.c
index e3dfd93c25..e6a9fe8111 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1101,7 +1101,7 @@  static void am_append_signoff(struct am_state *state)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
+	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len + 1);
 	append_signoff(&sb, 0, 0);
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }