Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Sep 25, 2015
1 parent db85a8a commit 5096d49
Show file tree
Hide file tree
Showing 20 changed files with 52 additions and 47 deletions.
2 changes: 1 addition & 1 deletion archive-tar.c
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion builtin/gc.c
Expand Up @@ -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,
Expand Down
11 changes: 6 additions & 5 deletions builtin/init-db.c
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
Expand Down
9 changes: 5 additions & 4 deletions builtin/ls-tree.c
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion builtin/merge-index.c
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion builtin/merge-recursive.c
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion builtin/read-tree.c
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion builtin/unpack-file.c
Expand Up @@ -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");
Expand Down
8 changes: 5 additions & 3 deletions compat/mingw.c
Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion compat/winansi.c
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion connect.c
Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion convert.c
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions daemon.c
Expand Up @@ -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, "<unknown>");
xsnprintf(ip, sizeof(ip), "<unknown>");
}
return ip;
}
Expand All @@ -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;
Expand Down
12 changes: 6 additions & 6 deletions diff.c
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion http-push.c
Expand Up @@ -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");

Expand Down
6 changes: 3 additions & 3 deletions http.c
Expand Up @@ -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: ");

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions ll-merge.c
Expand Up @@ -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");
Expand Down Expand Up @@ -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);

Expand Down
8 changes: 4 additions & 4 deletions refs.c
Expand Up @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions sideband.c
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions strbuf.c
Expand Up @@ -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);
}
Expand Down

0 comments on commit 5096d49

Please sign in to comment.