From 5096d4909f9b13c7a650d9dbb7c9702ea7413566 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:06:08 -0400 Subject: [PATCH] convert trivial sprintf / strcpy calls to xsnprintf We sometimes sprintf into fixed-size buffers when we know that the buffer is large enough to fit the input (either because it's a constant, or because it's numeric input that is bounded in size). Likewise with strcpy of constant strings. However, these sites make it hard to audit sprintf and strcpy calls for buffer overflows, as a reader has to cross-reference the size of the array with the input. Let's use xsnprintf instead, which communicates to a reader that we don't expect this to overflow (and catches the mistake in case we do). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- archive-tar.c | 2 +- builtin/gc.c | 2 +- builtin/init-db.c | 11 ++++++----- builtin/ls-tree.c | 9 +++++---- builtin/merge-index.c | 2 +- builtin/merge-recursive.c | 2 +- builtin/read-tree.c | 2 +- builtin/unpack-file.c | 2 +- compat/mingw.c | 8 +++++--- compat/winansi.c | 2 +- connect.c | 2 +- convert.c | 3 ++- daemon.c | 4 ++-- diff.c | 12 ++++++------ http-push.c | 2 +- http.c | 6 +++--- ll-merge.c | 12 ++++++------ refs.c | 8 ++++---- sideband.c | 4 ++-- strbuf.c | 4 ++-- 20 files changed, 52 insertions(+), 47 deletions(-) diff --git a/archive-tar.c b/archive-tar.c index b6b30bb57..d543f93fc 100644 --- a/archive-tar.c +++ b/archive-tar.c @@ -301,7 +301,7 @@ static int write_global_extended_header(struct archiver_args *args) memset(&header, 0, sizeof(header)); *header.typeflag = TYPEFLAG_GLOBAL_HEADER; mode = 0100666; - strcpy(header.name, "pax_global_header"); + xsnprintf(header.name, sizeof(header.name), "pax_global_header"); prepare_header(args, &header, mode, ext_header.len); write_blocked(&header, sizeof(header)); write_blocked(ext_header.buf, ext_header.len); diff --git a/builtin/gc.c b/builtin/gc.c index 0ad8d30b5..57584bc99 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -194,7 +194,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) return NULL; if (gethostname(my_host, sizeof(my_host))) - strcpy(my_host, "unknown"); + xsnprintf(my_host, sizeof(my_host), "unknown"); pidfile_path = git_pathdup("gc.pid"); fd = hold_lock_file_for_update(&lock, pidfile_path, diff --git a/builtin/init-db.c b/builtin/init-db.c index 69323e186..e7d0e31f4 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -262,7 +262,8 @@ static int create_default_files(const char *template_path) } /* This forces creation of new config file */ - sprintf(repo_version_string, "%d", GIT_REPO_VERSION); + xsnprintf(repo_version_string, sizeof(repo_version_string), + "%d", GIT_REPO_VERSION); git_config_set("core.repositoryformatversion", repo_version_string); path[len] = 0; @@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags) */ if (shared_repository < 0) /* force to the mode value */ - sprintf(buf, "0%o", -shared_repository); + xsnprintf(buf, sizeof(buf), "0%o", -shared_repository); else if (shared_repository == PERM_GROUP) - sprintf(buf, "%d", OLD_PERM_GROUP); + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP); else if (shared_repository == PERM_EVERYBODY) - sprintf(buf, "%d", OLD_PERM_EVERYBODY); + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY); else - die("oops"); + die("BUG: invalid value for shared_repository"); git_config_set("core.sharedrepository", buf); git_config_set("receive.denyNonFastforwards", "true"); } diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3b04a0f08..0e30d8623 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -96,12 +96,13 @@ static int show_tree(const unsigned char *sha1, struct strbuf *base, if (!strcmp(type, blob_type)) { unsigned long size; if (sha1_object_info(sha1, &size) == OBJ_BAD) - strcpy(size_text, "BAD"); + xsnprintf(size_text, sizeof(size_text), + "BAD"); else - snprintf(size_text, sizeof(size_text), - "%lu", size); + xsnprintf(size_text, sizeof(size_text), + "%lu", size); } else - strcpy(size_text, "-"); + xsnprintf(size_text, sizeof(size_text), "-"); printf("%06o %s %s %7s\t", mode, type, find_unique_abbrev(sha1, abbrev), size_text); diff --git a/builtin/merge-index.c b/builtin/merge-index.c index 1a1eafa6f..1d6611191 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -23,7 +23,7 @@ static int merge_entry(int pos, const char *path) break; found++; strcpy(hexbuf[stage], sha1_to_hex(ce->sha1)); - sprintf(ownbuf[stage], "%o", ce->ce_mode); + xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode); arguments[stage] = hexbuf[stage]; arguments[stage + 4] = ownbuf[stage]; } while (++pos < active_nr); diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index a90f28f34..491efd556 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -14,7 +14,7 @@ static const char *better_branch_name(const char *branch) if (strlen(branch) != 40) return branch; - sprintf(githead_env, "GITHEAD_%s", branch); + xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch); name = getenv(githead_env); return name ? name : branch; } diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 2379e1106..8c693e756 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -90,7 +90,7 @@ static int debug_merge(const struct cache_entry * const *stages, debug_stage("index", stages[0], o); for (i = 1; i <= o->merge_size; i++) { char buf[24]; - sprintf(buf, "ent#%d", i); + xsnprintf(buf, sizeof(buf), "ent#%d", i); debug_stage(buf, stages[i], o); } return 0; diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c index 19200291a..6fc6bcdf7 100644 --- a/builtin/unpack-file.c +++ b/builtin/unpack-file.c @@ -12,7 +12,7 @@ static char *create_temp_file(unsigned char *sha1) if (!buf || type != OBJ_BLOB) die("unable to read blob object %s", sha1_to_hex(sha1)); - strcpy(path, ".merge_file_XXXXXX"); + xsnprintf(path, sizeof(path), ".merge_file_XXXXXX"); fd = xmkstemp(path); if (write_in_full(fd, buf, size) != size) die_errno("unable to write temp-file"); diff --git a/compat/mingw.c b/compat/mingw.c index f74da235f..a168800ae 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2133,9 +2133,11 @@ int uname(struct utsname *buf) { DWORD v = GetVersion(); memset(buf, 0, sizeof(*buf)); - strcpy(buf->sysname, "Windows"); - sprintf(buf->release, "%u.%u", v & 0xff, (v >> 8) & 0xff); + xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows"); + xsnprintf(buf->release, sizeof(buf->release), + "%u.%u", v & 0xff, (v >> 8) & 0xff); /* assuming NT variants only.. */ - sprintf(buf->version, "%u", (v >> 16) & 0x7fff); + xsnprintf(buf->version, sizeof(buf->version), + "%u", (v >> 16) & 0x7fff); return 0; } diff --git a/compat/winansi.c b/compat/winansi.c index efc5bb3a4..ceff55bd6 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -539,7 +539,7 @@ void winansi_init(void) return; /* create a named pipe to communicate with the console thread */ - sprintf(name, "\\\\.\\pipe\\winansi%lu", GetCurrentProcessId()); + xsnprintf(name, sizeof(name), "\\\\.\\pipe\\winansi%lu", GetCurrentProcessId()); hwrite = CreateNamedPipe(name, PIPE_ACCESS_OUTBOUND, PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL); if (hwrite == INVALID_HANDLE_VALUE) diff --git a/connect.c b/connect.c index c0144d859..1d5c5e005 100644 --- a/connect.c +++ b/connect.c @@ -332,7 +332,7 @@ static const char *ai_name(const struct addrinfo *ai) static char addr[NI_MAXHOST]; if (getnameinfo(ai->ai_addr, ai->ai_addrlen, addr, sizeof(addr), NULL, 0, NI_NUMERICHOST) != 0) - strcpy(addr, "(unknown)"); + xsnprintf(addr, sizeof(addr), "(unknown)"); return addr; } diff --git a/convert.c b/convert.c index f3bd3e93f..814e81443 100644 --- a/convert.c +++ b/convert.c @@ -1289,7 +1289,8 @@ static struct stream_filter *ident_filter(const unsigned char *sha1) { struct ident_filter *ident = xmalloc(sizeof(*ident)); - sprintf(ident->ident, ": %s $", sha1_to_hex(sha1)); + xsnprintf(ident->ident, sizeof(ident->ident), + ": %s $", sha1_to_hex(sha1)); strbuf_init(&ident->left, 0); ident->filter.vtbl = &ident_vtbl; ident->state = 0; diff --git a/daemon.c b/daemon.c index f9eb29688..5218a3f1c 100644 --- a/daemon.c +++ b/daemon.c @@ -901,7 +901,7 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len) inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len); break; default: - strcpy(ip, ""); + xsnprintf(ip, sizeof(ip), ""); } return ip; } @@ -916,7 +916,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis int gai; long flags; - sprintf(pbuf, "%d", listen_port); + xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port); memset(&hints, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC; hints.ai_socktype = SOCK_STREAM; diff --git a/diff.c b/diff.c index 08508f6a2..788e371de 100644 --- a/diff.c +++ b/diff.c @@ -2880,7 +2880,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, temp->name = get_tempfile_path(&temp->tempfile); strcpy(temp->hex, sha1_to_hex(sha1)); temp->hex[40] = 0; - sprintf(temp->mode, "%06o", mode); + xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode); strbuf_release(&buf); strbuf_release(&template); free(path_dup); @@ -2897,8 +2897,8 @@ static struct diff_tempfile *prepare_temp_file(const char *name, * a '+' entry produces this for file-1. */ temp->name = "/dev/null"; - strcpy(temp->hex, "."); - strcpy(temp->mode, "."); + xsnprintf(temp->hex, sizeof(temp->hex), "."); + xsnprintf(temp->mode, sizeof(temp->mode), "."); return temp; } @@ -2935,7 +2935,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name, * !(one->sha1_valid), as long as * DIFF_FILE_VALID(one). */ - sprintf(temp->mode, "%06o", one->mode); + xsnprintf(temp->mode, sizeof(temp->mode), "%06o", one->mode); } return temp; } @@ -4081,9 +4081,9 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len) if (abblen < 37) { static char hex[41]; if (len < abblen && abblen <= len + 2) - sprintf(hex, "%s%.*s", abbrev, len+3-abblen, ".."); + xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, ".."); else - sprintf(hex, "%s...", abbrev); + xsnprintf(hex, sizeof(hex), "%s...", abbrev); return hex; } return sha1_to_hex(sha1); diff --git a/http-push.c b/http-push.c index c98dad23d..154e67bb0 100644 --- a/http-push.c +++ b/http-push.c @@ -881,7 +881,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped); free(escaped); - sprintf(timeout_header, "Timeout: Second-%ld", timeout); + xsnprintf(timeout_header, sizeof(timeout_header), "Timeout: Second-%ld", timeout); dav_headers = curl_slist_append(dav_headers, timeout_header); dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml"); diff --git a/http.c b/http.c index 9dce38025..7b0225996 100644 --- a/http.c +++ b/http.c @@ -1104,7 +1104,7 @@ static void write_accept_language(struct strbuf *buf) decimal_places++, max_q *= 10) ; - sprintf(q_format, ";q=0.%%0%dd", decimal_places); + xsnprintf(q_format, sizeof(q_format), ";q=0.%%0%dd", decimal_places); strbuf_addstr(buf, "Accept-Language: "); @@ -1601,7 +1601,7 @@ struct http_pack_request *new_http_pack_request( fprintf(stderr, "Resuming fetch of pack %s at byte %ld\n", sha1_to_hex(target->sha1), prev_posn); - sprintf(range, "Range: bytes=%ld-", prev_posn); + xsnprintf(range, sizeof(range), "Range: bytes=%ld-", prev_posn); preq->range_header = curl_slist_append(NULL, range); curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->range_header); @@ -1761,7 +1761,7 @@ struct http_object_request *new_http_object_request(const char *base_url, fprintf(stderr, "Resuming fetch of object %s at byte %ld\n", hex, prev_posn); - sprintf(range, "Range: bytes=%ld-", prev_posn); + xsnprintf(range, sizeof(range), "Range: bytes=%ld-", prev_posn); range_header = curl_slist_append(range_header, range); curl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, range_header); diff --git a/ll-merge.c b/ll-merge.c index fc3c04959..56f73b393 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -142,11 +142,11 @@ static struct ll_merge_driver ll_merge_drv[] = { { "union", "built-in union merge", ll_union_merge }, }; -static void create_temp(mmfile_t *src, char *path) +static void create_temp(mmfile_t *src, char *path, size_t len) { int fd; - strcpy(path, ".merge_file_XXXXXX"); + xsnprintf(path, len, ".merge_file_XXXXXX"); fd = xmkstemp(path); if (write_in_full(fd, src->ptr, src->size) != src->size) die_errno("unable to write temp-file"); @@ -187,10 +187,10 @@ static int ll_ext_merge(const struct ll_merge_driver *fn, result->ptr = NULL; result->size = 0; - create_temp(orig, temp[0]); - create_temp(src1, temp[1]); - create_temp(src2, temp[2]); - sprintf(temp[3], "%d", marker_size); + create_temp(orig, temp[0], sizeof(temp[0])); + create_temp(src1, temp[1], sizeof(temp[1])); + create_temp(src2, temp[2], sizeof(temp[2])); + xsnprintf(temp[3], sizeof(temp[3]), "%d", marker_size); strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict); diff --git a/refs.c b/refs.c index 4e15f60d9..d5c8b2f51 100644 --- a/refs.c +++ b/refs.c @@ -3326,10 +3326,10 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, msglen = msg ? strlen(msg) : 0; maxlen = strlen(committer) + msglen + 100; logrec = xmalloc(maxlen); - len = sprintf(logrec, "%s %s %s\n", - sha1_to_hex(old_sha1), - sha1_to_hex(new_sha1), - committer); + len = xsnprintf(logrec, maxlen, "%s %s %s\n", + sha1_to_hex(old_sha1), + sha1_to_hex(new_sha1), + committer); if (msglen) len += copy_msg(logrec + len - 1, msg) - 1; diff --git a/sideband.c b/sideband.c index 7f9dc229f..fde8adc00 100644 --- a/sideband.c +++ b/sideband.c @@ -137,11 +137,11 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet if (packet_max - 5 < n) n = packet_max - 5; if (0 <= band) { - sprintf(hdr, "%04x", n + 5); + xsnprintf(hdr, sizeof(hdr), "%04x", n + 5); hdr[4] = band; write_or_die(fd, hdr, 5); } else { - sprintf(hdr, "%04x", n + 4); + xsnprintf(hdr, sizeof(hdr), "%04x", n + 4); write_or_die(fd, hdr, 4); } write_or_die(fd, p, n); diff --git a/strbuf.c b/strbuf.c index f3c44fb3b..107c45d29 100644 --- a/strbuf.c +++ b/strbuf.c @@ -245,8 +245,8 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size static char prefix2[2]; if (prefix1[0] != comment_line_char) { - sprintf(prefix1, "%c ", comment_line_char); - sprintf(prefix2, "%c", comment_line_char); + xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char); + xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char); } add_lines(out, prefix1, prefix2, buf, size); }