@@ -110,14 +110,14 @@ void strbuf_release(struct strbuf *sb);
char *strbuf_detach(struct strbuf *sb, size_t *sz);
/**
- * Attach a string to a buffer. You should specify the string to attach,
- * the current length of the string and the amount of allocated memory.
- * The amount must be larger than the string length, because the string you
- * pass is supposed to be a NUL-terminated string. This string _must_ be
- * malloc()ed, and after attaching, the pointer cannot be relied upon
- * anymore, and neither be free()d directly.
- */
-void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);
+ * Attach a string to a buffer. You should specify the string to attach
+ * and its length. The amount of allocated memory will be assumed to be
+ * one greater than the length. The string you pass is supposed to be a
+ * NUL-terminated string. This string _must_ be malloc()ed, and after
+ * attaching, the pointer cannot be relied upon anymore, nor should it
+ * be free()d directly.
+ */
+void strbuf_attach(struct strbuf *sb, void *str, size_t len);
/**
* Swap the contents of two string buffers.
@@ -3251,7 +3251,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns
if (!result)
return -1;
/* XXX read_sha1_file NUL-terminates */
- strbuf_attach(buf, result, sz, sz + 1);
+ strbuf_attach(buf, result, sz);
}
return 0;
}
@@ -89,7 +89,7 @@ void *object_file_to_archive(const struct archiver_args *args,
struct strbuf buf = STRBUF_INIT;
size_t size = 0;
- strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
+ strbuf_attach(&buf, buffer, *sizep);
convert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);
if (commit)
format_subst(commit, buf.buf, buf.len, &buf);
@@ -241,7 +241,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,
case S_IFREG:
if (opt->flags.allow_textconv &&
textconv_object(r, read_from, mode, &null_oid, 0, &buf_ptr, &buf_len))
- strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);
+ strbuf_attach(&buf, buf_ptr, buf_len);
else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
die_errno("cannot open or read '%s'", read_from);
break;
@@ -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 + 1);
+ strbuf_attach(&sb, state->msg, state->msg_len);
append_signoff(&sb, 0, 0);
state->msg = strbuf_detach(&sb, &state->msg_len);
}
@@ -467,7 +467,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,
free(re_src);
}
- strbuf_attach(buf, dst, dst_len, dst_len + 1);
+ strbuf_attach(buf, dst, dst_len);
return 1;
}
@@ -492,7 +492,7 @@ static int encode_to_worktree(const char *path, const char *src, size_t src_len,
return 0;
}
- strbuf_attach(buf, dst, dst_len, dst_len + 1);
+ strbuf_attach(buf, dst, dst_len);
return 1;
}
@@ -1212,7 +1212,7 @@ static void lf_to_crlf(struct strbuf *msg)
new_msg[j++] = '\r';
lastc = new_msg[j++] = msg->buf[i];
}
- strbuf_attach(msg, new_msg, j, j + 1);
+ strbuf_attach(msg, new_msg, j);
}
/*
@@ -459,7 +459,7 @@ static int convert_to_utf8(struct mailinfo *mi,
return error("cannot convert from %s to %s",
charset, mi->metainfo_charset);
}
- strbuf_attach(line, out, strlen(out), strlen(out) + 1);
+ strbuf_attach(line, out, strlen(out));
return 0;
}
@@ -2963,7 +2963,7 @@ static int read_oid_strbuf(struct merge_options *opt,
free(buf);
return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
}
- strbuf_attach(dst, buf, size, size + 1);
+ strbuf_attach(dst, buf, size);
return 0;
}
@@ -815,8 +815,7 @@ const char *enter_repo(const char *path, int strict)
char *newpath = expand_user_path(used_path.buf, 0);
if (!newpath)
return NULL;
- strbuf_attach(&used_path, newpath, strlen(newpath),
- strlen(newpath) + 1);
+ strbuf_attach(&used_path, newpath, strlen(newpath));
}
for (i = 0; suffix[i]; i++) {
struct stat st;
@@ -590,7 +590,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)
return buf; /* should not happen but be defensive */
len = cp + 1 - (buf + start);
- strbuf_attach(&tmp, buf, strlen(buf), strlen(buf) + 1);
+ strbuf_attach(&tmp, buf, strlen(buf));
if (is_encoding_utf8(encoding)) {
/* we have re-coded to UTF-8; drop the header */
strbuf_remove(&tmp, start, len);
@@ -1687,7 +1687,7 @@ void repo_format_commit_message(struct repository *r,
char *out = reencode_string_len(sb->buf, sb->len,
output_enc, utf8, &outsz);
if (out)
- strbuf_attach(sb, out, outsz, outsz + 1);
+ strbuf_attach(sb, out, outsz);
}
free(context.commit_encoding);
@@ -1514,7 +1514,7 @@ static int commit_ref(struct ref_lock *lock)
size_t len = strlen(path);
struct strbuf sb_path = STRBUF_INIT;
- strbuf_attach(&sb_path, path, len, len + 1);
+ strbuf_attach(&sb_path, path, len);
/*
* If this fails, commit_lock_file() will also fail
@@ -77,13 +77,16 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
return res;
}
-void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
+void strbuf_attach(struct strbuf *sb, void *buf, size_t len)
{
+ if (!buf)
+ BUG("NULL-buffer in strbuf_attach");
+ if (unsigned_add_overflows(len, 1))
+ die("you want to use way too much memory");
strbuf_release(sb);
sb->buf = buf;
sb->len = len;
- sb->alloc = alloc;
- strbuf_grow(sb, 0);
+ sb->alloc = len + 1;
sb->buf[sb->len] = '\0';
}
@@ -152,7 +155,7 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
if (!out)
return -1;
- strbuf_attach(sb, out, len, len + 1);
+ strbuf_attach(sb, out, len);
return 0;
}
@@ -1095,8 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
for (ptr = trailer_lines; *ptr; ptr++) {
if (last && isspace((*ptr)->buf[0])) {
struct strbuf sb = STRBUF_INIT;
- strbuf_attach(&sb, *last, strlen(*last),
- strlen(*last) + 1);
+ strbuf_attach(&sb, *last, strlen(*last));
strbuf_addbuf(&sb, *ptr);
*last = strbuf_detach(&sb, NULL);
continue;
The last few commits have fixed a decent number of call sites that pass in the same value for `alloc` and `len` when attaching a string to a strbuf, despite the documentation being quite clear about this not being ok. As a result, all users currently pass in `len + 1`. Because this interface was used incorrectly (albeit not necessarily insecurely) before, simplify it by making `strbuf_attach()` simply assume that the allocated length is exactly `len + 1`. If and when we do gain one or two call sites that know better, we can either keep on using this pessimistic assumption and it might not actually hurt much, or we could give them a dedicated function similar to the current more flexible (and proven error-prone) `strbuf_attach()`. This means we can drop the call to `strbuf_grow()`. It really only served to do the alloc+copy+free dance that we were supposedly avoiding. That said, we should consider the edge case of `buf == NULL`. Let's check for this and BUG. Even for a zero-length string, you need somewhere to stick the trailing NUL. (Also: If someone used to pass `alloc == len` for the case `len == 0`, we used to end up leaking `buf` on the assumption that we were using `strbuf_slopbuf`. Not the biggest leak ever, but still.) Finally, one might wonder if we could go further and make `strbuf_attach()` call `strlen()`. Some call sites wouldn't mind, but others do have the length already and we might as well use it. Those that don't have the length will figure it out as they call this function. Before this commit, they'd do two calls to `strlen()` so they either didn't care much or they trusted the compiler to optimize for them. They're not any worse off now. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- strbuf.h | 16 ++++++++-------- apply.c | 2 +- archive.c | 2 +- blame.c | 2 +- builtin/am.c | 2 +- convert.c | 4 ++-- imap-send.c | 2 +- mailinfo.c | 2 +- merge-recursive.c | 2 +- path.c | 3 +-- pretty.c | 4 ++-- refs/files-backend.c | 2 +- strbuf.c | 11 +++++++---- trailer.c | 3 +-- 14 files changed, 29 insertions(+), 28 deletions(-)