Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
use xmallocz to avoid size arithmetic
We frequently allocate strings as xmalloc(len + 1), where
the extra 1 is for the NUL terminator. This can be done more
simply with xmallocz, which also checks for integer
overflow.

There's no case where switching xmalloc(n+1) to xmallocz(n)
is wrong; the result is the same length, and malloc made no
guarantees about what was in the buffer anyway. But in some
cases, we can stop manually placing NUL at the end of the
allocated buffer. But that's only safe if it's clear that
the contents will always fill the buffer.

In each case where this patch does so, I manually examined
the control flow, and I tried to err on the side of caution.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Feb 22, 2016
1 parent b32fa95 commit 3733e69
Show file tree
Hide file tree
Showing 15 changed files with 17 additions and 25 deletions.
2 changes: 1 addition & 1 deletion builtin/check-ref-format.c
Expand Up @@ -20,7 +20,7 @@ static const char builtin_check_ref_format_usage[] =
*/
static char *collapse_slashes(const char *refname)
{
char *ret = xmalloc(strlen(refname) + 1);
char *ret = xmallocz(strlen(refname));
char ch;
char prev = '/';
char *cp = ret;
Expand Down
2 changes: 1 addition & 1 deletion builtin/merge-tree.c
Expand Up @@ -174,7 +174,7 @@ static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsi

static char *traverse_path(const struct traverse_info *info, const struct name_entry *n)
{
char *path = xmalloc(traverse_path_len(info, n) + 1);
char *path = xmallocz(traverse_path_len(info, n));
return make_traverse_path(path, info, n);
}

Expand Down
2 changes: 1 addition & 1 deletion builtin/worktree.c
Expand Up @@ -52,7 +52,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)
return 1;
}
len = st.st_size;
path = xmalloc(len + 1);
path = xmallocz(len);
read_in_full(fd, path, len);
close(fd);
while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
Expand Down
3 changes: 1 addition & 2 deletions column.c
Expand Up @@ -173,9 +173,8 @@ static void display_table(const struct string_list *list,
if (colopts & COL_DENSE)
shrink_columns(&data);

empty_cell = xmalloc(initial_width + 1);
empty_cell = xmallocz(initial_width);
memset(empty_cell, ' ', initial_width);
empty_cell[initial_width] = '\0';
for (y = 0; y < data.rows; y++) {
for (x = 0; x < data.cols; x++)
if (display_cell(&data, initial_width, empty_cell, x, y))
Expand Down
4 changes: 1 addition & 3 deletions combine-diff.c
Expand Up @@ -1043,16 +1043,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
elem->mode = canon_mode(S_IFLNK);

result_size = len;
result = xmalloc(len + 1);
result = xmallocz(len);

done = read_in_full(fd, result, len);
if (done < 0)
die_errno("read error '%s'", elem->path);
else if (done < len)
die("early EOF '%s'", elem->path);

result[len] = 0;

/* If not a fake symlink, apply filters, e.g. autocrlf */
if (is_file) {
struct strbuf buf = STRBUF_INIT;
Expand Down
4 changes: 1 addition & 3 deletions config.c
Expand Up @@ -1878,7 +1878,7 @@ static int git_config_parse_key_1(const char *key, char **store_key, int *basele
* Validate the key and while at it, lower case it for matching.
*/
if (store_key)
*store_key = xmalloc(strlen(key) + 1);
*store_key = xmallocz(strlen(key));

dot = 0;
for (i = 0; key[i]; i++) {
Expand All @@ -1902,8 +1902,6 @@ static int git_config_parse_key_1(const char *key, char **store_key, int *basele
if (store_key)
(*store_key)[i] = c;
}
if (store_key)
(*store_key)[i] = 0;

return 0;

Expand Down
2 changes: 1 addition & 1 deletion dir.c
Expand Up @@ -711,7 +711,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
close(fd);
return 0;
}
buf = xmalloc(size+1);
buf = xmallocz(size);
if (read_in_full(fd, buf, size) != size) {
free(buf);
close(fd);
Expand Down
2 changes: 1 addition & 1 deletion entry.c
Expand Up @@ -6,7 +6,7 @@
static void create_directories(const char *path, int path_len,
const struct checkout *state)
{
char *buf = xmalloc(path_len + 1);
char *buf = xmallocz(path_len);
int len = 0;

while (len < path_len) {
Expand Down
3 changes: 1 addition & 2 deletions grep.c
Expand Up @@ -1741,15 +1741,14 @@ static int grep_source_load_file(struct grep_source *gs)
i = open(filename, O_RDONLY);
if (i < 0)
goto err_ret;
data = xmalloc(size + 1);
data = xmallocz(size);
if (st.st_size != read_in_full(i, data, size)) {
error(_("'%s': short read %s"), filename, strerror(errno));
close(i);
free(data);
return -1;
}
close(i);
data[size] = 0;

gs->buf = data;
gs->size = size;
Expand Down
5 changes: 2 additions & 3 deletions imap-send.c
Expand Up @@ -892,12 +892,11 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
response = xstrfmt("%s %s", user, hex);
resp_len = strlen(response) + 1;

response_64 = xmalloc(ENCODED_SIZE(resp_len) + 1);
response_64 = xmallocz(ENCODED_SIZE(resp_len));
encoded_len = EVP_EncodeBlock((unsigned char *)response_64,
(unsigned char *)response, resp_len);
if (encoded_len < 0)
die("EVP_EncodeBlock error");
response_64[encoded_len] = '\0';
return (char *)response_64;
}

Expand Down Expand Up @@ -1188,7 +1187,7 @@ static void lf_to_crlf(struct strbuf *msg)
j++;
}

new = xmalloc(j + 1);
new = xmallocz(j);

/*
* Second pass: write the new string. Note that this loop is
Expand Down
2 changes: 1 addition & 1 deletion ll-merge.c
Expand Up @@ -205,7 +205,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
if (fstat(fd, &st))
goto close_bad;
result->size = st.st_size;
result->ptr = xmalloc(result->size + 1);
result->ptr = xmallocz(result->size);
if (read_in_full(fd, result->ptr, result->size) != result->size) {
free(result->ptr);
result->ptr = NULL;
Expand Down
2 changes: 1 addition & 1 deletion progress.c
Expand Up @@ -247,7 +247,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
size_t len = strlen(msg) + 5;
struct throughput *tp = progress->throughput;

bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
Expand Down
2 changes: 1 addition & 1 deletion refs.c
Expand Up @@ -124,7 +124,7 @@ int refname_is_safe(const char *refname)
char *buf;
int result;

buf = xmalloc(strlen(refname) + 1);
buf = xmallocz(strlen(refname));
/*
* Does the refname try to escape refs/?
* For example: refs/foo/../bar is safe but refs/foo/../../bar
Expand Down
5 changes: 2 additions & 3 deletions setup.c
Expand Up @@ -88,7 +88,7 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
sanitized = xmalloc(strlen(path) + 1);
sanitized = xmallocz(strlen(path));
if (remaining_prefix)
*remaining_prefix = 0;
if (normalize_path_copy_len(sanitized, path, remaining_prefix)) {
Expand Down Expand Up @@ -499,14 +499,13 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
error_code = READ_GITFILE_ERR_OPEN_FAILED;
goto cleanup_return;
}
buf = xmalloc(st.st_size + 1);
buf = xmallocz(st.st_size);
len = read_in_full(fd, buf, st.st_size);
close(fd);
if (len != st.st_size) {
error_code = READ_GITFILE_ERR_READ_FAILED;
goto cleanup_return;
}
buf[len] = '\0';
if (!starts_with(buf, "gitdir: ")) {
error_code = READ_GITFILE_ERR_INVALID_FORMAT;
goto cleanup_return;
Expand Down
2 changes: 1 addition & 1 deletion strbuf.c
Expand Up @@ -685,7 +685,7 @@ char *xstrdup_tolower(const char *string)
size_t len, i;

len = strlen(string);
result = xmalloc(len + 1);
result = xmallocz(len);
for (i = 0; i < len; i++)
result[i] = tolower(string[i]);
result[i] = '\0';
Expand Down

0 comments on commit 3733e69

Please sign in to comment.