@matheusbsbs / matheus-bagliano / commits / 5096d4909f

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>

Jeff King committed Sep 24, 2015 at 17:06 UTC 5096d4909f9b13c7a650d9dbb7c9702ea7413566

20 files changed +52 -47

archive-tar.c

+1 -1

@@ -301,7 +301,7 @@ static int write_global_extended_header(struct archiver_args *args)

301 memset(&header, 0, sizeof(header));

302 *header.typeflag = TYPEFLAG_GLOBAL_HEADER;

303 mode = 0100666;

304 - strcpy(header.name, "pax_global_header");

304 + xsnprintf(header.name, sizeof(header.name), "pax_global_header");

305 prepare_header(args, &header, mode, ext_header.len);

306 write_blocked(&header, sizeof(header));

307 write_blocked(ext_header.buf, ext_header.len);

builtin/gc.c

+1 -1

@@ -194,7 +194,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)

194 return NULL;

195

196 if (gethostname(my_host, sizeof(my_host)))

197 - strcpy(my_host, "unknown");

197 + xsnprintf(my_host, sizeof(my_host), "unknown");

198

199 pidfile_path = git_pathdup("gc.pid");

200 fd = hold_lock_file_for_update(&lock, pidfile_path,

builtin/init-db.c

+6 -5

@@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)

262 }

263

264 /* This forces creation of new config file */

265 - sprintf(repo_version_string, "%d", GIT_REPO_VERSION);

265 + xsnprintf(repo_version_string, sizeof(repo_version_string),

266 + "%d", GIT_REPO_VERSION);

267 git_config_set("core.repositoryformatversion", repo_version_string);

268

269 path[len] = 0;

@@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags)

415 */

416 if (shared_repository < 0)

417 /* force to the mode value */

417 - sprintf(buf, "0%o", -shared_repository);

418 + xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);

419 else if (shared_repository == PERM_GROUP)

419 - sprintf(buf, "%d", OLD_PERM_GROUP);

420 + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);

421 else if (shared_repository == PERM_EVERYBODY)

421 - sprintf(buf, "%d", OLD_PERM_EVERYBODY);

422 + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);

423 else

423 - die("oops");

424 + die("BUG: invalid value for shared_repository");

425 git_config_set("core.sharedrepository", buf);

426 git_config_set("receive.denyNonFastforwards", "true");

427 }

builtin/ls-tree.c

+5 -4

@@ -96,12 +96,13 @@ static int show_tree(const unsigned char *sha1, struct strbuf *base,

96 if (!strcmp(type, blob_type)) {

97 unsigned long size;

98 if (sha1_object_info(sha1, &size) == OBJ_BAD)

99 - strcpy(size_text, "BAD");

99 + xsnprintf(size_text, sizeof(size_text),

100 + "BAD");

101 else

101 - snprintf(size_text, sizeof(size_text),

102 - "%lu", size);

102 + xsnprintf(size_text, sizeof(size_text),

103 + "%lu", size);

104 } else

104 - strcpy(size_text, "-");

105 + xsnprintf(size_text, sizeof(size_text), "-");

106 printf("%06o %s %s %7s\t", mode, type,

107 find_unique_abbrev(sha1, abbrev),

108 size_text);

builtin/merge-index.c

+1 -1

@@ -23,7 +23,7 @@ static int merge_entry(int pos, const char *path)

23 break;

24 found++;

25 strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));

26 - sprintf(ownbuf[stage], "%o", ce->ce_mode);

26 + xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);

27 arguments[stage] = hexbuf[stage];

28 arguments[stage + 4] = ownbuf[stage];

29 } while (++pos < active_nr);

builtin/merge-recursive.c

+1 -1

@@ -14,7 +14,7 @@ static const char *better_branch_name(const char *branch)

15 if (strlen(branch) != 40)

16 return branch;

17 - sprintf(githead_env, "GITHEAD_%s", branch);

17 + xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);

18 name = getenv(githead_env);

19 return name ? name : branch;

20 }

builtin/read-tree.c

+1 -1

@@ -90,7 +90,7 @@ static int debug_merge(const struct cache_entry * const *stages,

90 debug_stage("index", stages[0], o);

91 for (i = 1; i <= o->merge_size; i++) {

92 char buf[24];

93 - sprintf(buf, "ent#%d", i);

93 + xsnprintf(buf, sizeof(buf), "ent#%d", i);

94 debug_stage(buf, stages[i], o);

95 }

96 return 0;

builtin/unpack-file.c

+1 -1

@@ -12,7 +12,7 @@ static char *create_temp_file(unsigned char *sha1)

12 if (!buf || type != OBJ_BLOB)

13 die("unable to read blob object %s", sha1_to_hex(sha1));

15 - strcpy(path, ".merge_file_XXXXXX");

15 + xsnprintf(path, sizeof(path), ".merge_file_XXXXXX");

16 fd = xmkstemp(path);

17 if (write_in_full(fd, buf, size) != size)

18 die_errno("unable to write temp-file");

compat/mingw.c

+5 -3

@@ -2133,9 +2133,11 @@ int uname(struct utsname *buf)

2133 {

2134 DWORD v = GetVersion();

2135 memset(buf, 0, sizeof(*buf));

2136 - strcpy(buf->sysname, "Windows");

2137 - sprintf(buf->release, "%u.%u", v & 0xff, (v >> 8) & 0xff);

2136 + xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");

2137 + xsnprintf(buf->release, sizeof(buf->release),

2138 + "%u.%u", v & 0xff, (v >> 8) & 0xff);

2139 /* assuming NT variants only.. */

2139 - sprintf(buf->version, "%u", (v >> 16) & 0x7fff);

2140 + xsnprintf(buf->version, sizeof(buf->version),

2141 + "%u", (v >> 16) & 0x7fff);

2142 return 0;

2143 }

compat/winansi.c

+1 -1

@@ -539,7 +539,7 @@ void winansi_init(void)

539 return;

540

541 /* create a named pipe to communicate with the console thread */

542 - sprintf(name, "\\\\.\\pipe\\winansi%lu", GetCurrentProcessId());

542 + xsnprintf(name, sizeof(name), "\\\\.\\pipe\\winansi%lu", GetCurrentProcessId());

543 hwrite = CreateNamedPipe(name, PIPE_ACCESS_OUTBOUND,

544 PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL);

545 if (hwrite == INVALID_HANDLE_VALUE)

connect.c

+1 -1

@@ -332,7 +332,7 @@ static const char *ai_name(const struct addrinfo *ai)

332 static char addr[NI_MAXHOST];

333 if (getnameinfo(ai->ai_addr, ai->ai_addrlen, addr, sizeof(addr), NULL, 0,

334 NI_NUMERICHOST) != 0)

335 - strcpy(addr, "(unknown)");

335 + xsnprintf(addr, sizeof(addr), "(unknown)");

336

337 return addr;

338 }

convert.c

+2 -1

@@ -1289,7 +1289,8 @@ static struct stream_filter *ident_filter(const unsigned char *sha1)

1289 {

1290 struct ident_filter *ident = xmalloc(sizeof(*ident));

1291

1292 - sprintf(ident->ident, ": %s $", sha1_to_hex(sha1));

1292 + xsnprintf(ident->ident, sizeof(ident->ident),

1293 + ": %s $", sha1_to_hex(sha1));

1294 strbuf_init(&ident->left, 0);

1295 ident->filter.vtbl = &ident_vtbl;

1296 ident->state = 0;

daemon.c

+2 -2

@@ -901,7 +901,7 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)

901 inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);

902 break;

903 default:

904 - strcpy(ip, "<unknown>");

904 + xsnprintf(ip, sizeof(ip), "<unknown>");

905 }

906 return ip;

907 }

@@ -916,7 +916,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis

916 int gai;

917 long flags;

918

919 - sprintf(pbuf, "%d", listen_port);

919 + xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);

920 memset(&hints, 0, sizeof(hints));

921 hints.ai_family = AF_UNSPEC;

922 hints.ai_socktype = SOCK_STREAM;

diff.c

+6 -6

@@ -2880,7 +2880,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,

2880 temp->name = get_tempfile_path(&temp->tempfile);

2881 strcpy(temp->hex, sha1_to_hex(sha1));

2882 temp->hex[40] = 0;

2883 - sprintf(temp->mode, "%06o", mode);

2883 + xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);

2884 strbuf_release(&buf);

2885 strbuf_release(&template);

2886 free(path_dup);

@@ -2897,8 +2897,8 @@ static struct diff_tempfile *prepare_temp_file(const char *name,

2897 * a '+' entry produces this for file-1.

2898 */

2899 temp->name = "/dev/null";

2900 - strcpy(temp->hex, ".");

2901 - strcpy(temp->mode, ".");

2900 + xsnprintf(temp->hex, sizeof(temp->hex), ".");

2901 + xsnprintf(temp->mode, sizeof(temp->mode), ".");

2902 return temp;

2903 }

2904

@@ -2935,7 +2935,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,

2935 * !(one->sha1_valid), as long as

2936 * DIFF_FILE_VALID(one).

2937 */

2938 - sprintf(temp->mode, "%06o", one->mode);

2938 + xsnprintf(temp->mode, sizeof(temp->mode), "%06o", one->mode);

2939 }

2940 return temp;

2941 }

@@ -4081,9 +4081,9 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)

4081 if (abblen < 37) {

4082 static char hex[41];

4083 if (len < abblen && abblen <= len + 2)

4084 - sprintf(hex, "%s%.*s", abbrev, len+3-abblen, "..");

4084 + xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");

4085 else

4086 - sprintf(hex, "%s...", abbrev);

4086 + xsnprintf(hex, sizeof(hex), "%s...", abbrev);

4087 return hex;

4088 }

4089 return sha1_to_hex(sha1);

http-push.c

+1 -1

@@ -881,7 +881,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)

881 strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);

882 free(escaped);

883

884 - sprintf(timeout_header, "Timeout: Second-%ld", timeout);

884 + xsnprintf(timeout_header, sizeof(timeout_header), "Timeout: Second-%ld", timeout);

885 dav_headers = curl_slist_append(dav_headers, timeout_header);

886 dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");

887

http.c

+3 -3

@@ -1104,7 +1104,7 @@ static void write_accept_language(struct strbuf *buf)

1104 decimal_places++, max_q *= 10)

1105 ;

1106

1107 - sprintf(q_format, ";q=0.%%0%dd", decimal_places);

1107 + xsnprintf(q_format, sizeof(q_format), ";q=0.%%0%dd", decimal_places);

1108

1109 strbuf_addstr(buf, "Accept-Language: ");

1110

@@ -1601,7 +1601,7 @@ struct http_pack_request *new_http_pack_request(

1601 fprintf(stderr,

1602 "Resuming fetch of pack %s at byte %ld\n",

1603 sha1_to_hex(target->sha1), prev_posn);

1604 - sprintf(range, "Range: bytes=%ld-", prev_posn);

1604 + xsnprintf(range, sizeof(range), "Range: bytes=%ld-", prev_posn);

1605 preq->range_header = curl_slist_append(NULL, range);

1606 curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,

1607 preq->range_header);

@@ -1761,7 +1761,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

1761 fprintf(stderr,

1762 "Resuming fetch of object %s at byte %ld\n",

1763 hex, prev_posn);

1764 - sprintf(range, "Range: bytes=%ld-", prev_posn);

1764 + xsnprintf(range, sizeof(range), "Range: bytes=%ld-", prev_posn);

1765 range_header = curl_slist_append(range_header, range);

1766 curl_easy_setopt(freq->slot->curl,

1767 CURLOPT_HTTPHEADER, range_header);

ll-merge.c

+6 -6

@@ -142,11 +142,11 @@ static struct ll_merge_driver ll_merge_drv[] = {

142 { "union", "built-in union merge", ll_union_merge },

143 };

144

145 -static void create_temp(mmfile_t *src, char *path)

145 +static void create_temp(mmfile_t *src, char *path, size_t len)

146 {

147 int fd;

148

149 - strcpy(path, ".merge_file_XXXXXX");

149 + xsnprintf(path, len, ".merge_file_XXXXXX");

150 fd = xmkstemp(path);

151 if (write_in_full(fd, src->ptr, src->size) != src->size)

152 die_errno("unable to write temp-file");

@@ -187,10 +187,10 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,

187

188 result->ptr = NULL;

189 result->size = 0;

190 - create_temp(orig, temp[0]);

191 - create_temp(src1, temp[1]);

192 - create_temp(src2, temp[2]);

193 - sprintf(temp[3], "%d", marker_size);

190 + create_temp(orig, temp[0], sizeof(temp[0]));

191 + create_temp(src1, temp[1], sizeof(temp[1]));

192 + create_temp(src2, temp[2], sizeof(temp[2]));

193 + xsnprintf(temp[3], sizeof(temp[3]), "%d", marker_size);

194

195 strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);

196

refs.c

+4 -4

@@ -3326,10 +3326,10 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,

3326 msglen = msg ? strlen(msg) : 0;

3327 maxlen = strlen(committer) + msglen + 100;

3328 logrec = xmalloc(maxlen);

3329 - len = sprintf(logrec, "%s %s %s\n",

3330 - sha1_to_hex(old_sha1),

3331 - sha1_to_hex(new_sha1),

3332 - committer);

3329 + len = xsnprintf(logrec, maxlen, "%s %s %s\n",

3330 + sha1_to_hex(old_sha1),

3331 + sha1_to_hex(new_sha1),

3332 + committer);

3333 if (msglen)

3334 len += copy_msg(logrec + len - 1, msg) - 1;

3335

sideband.c

+2 -2

@@ -137,11 +137,11 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet

137 if (packet_max - 5 < n)

138 n = packet_max - 5;

139 if (0 <= band) {

140 - sprintf(hdr, "%04x", n + 5);

140 + xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);

141 hdr[4] = band;

142 write_or_die(fd, hdr, 5);

143 } else {

144 - sprintf(hdr, "%04x", n + 4);

144 + xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);

145 write_or_die(fd, hdr, 4);

146 }

147 write_or_die(fd, p, n);

strbuf.c

+2 -2

@@ -245,8 +245,8 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size

245 static char prefix2[2];

246

247 if (prefix1[0] != comment_line_char) {

248 - sprintf(prefix1, "%c ", comment_line_char);

249 - sprintf(prefix2, "%c", comment_line_char);

248 + xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);

249 + xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);

250 }

251 add_lines(out, prefix1, prefix2, buf, size);

252 }