From ef1286d3c0ba714c6c2ae87e14edf3c462aef114 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:06:42 -0400 Subject: use xsnprintf for generating git object headers We generally use 32-byte buffers to format git's "type size" header fields. These should not generally overflow unless you can produce some truly gigantic objects (and our types come from our internal array of constant strings). But it is a good idea to use xsnprintf to make sure this is the case. Note that we slightly modify the interface to write_sha1_file_prepare, which nows uses "hdrlen" as an "in" parameter as well as an "out" (on the way in it stores the allocated size of the header, and on the way out it returns the ultimate size of the header). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fast-import.c') diff --git a/fast-import.c b/fast-import.c index 6c7c3c9b66..d0c25024cd 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1035,8 +1035,8 @@ static int store_object( git_SHA_CTX c; git_zstream s; - hdrlen = sprintf((char *)hdr,"%s %lu", typename(type), - (unsigned long)dat->len) + 1; + hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu", + typename(type), (unsigned long)dat->len) + 1; git_SHA1_Init(&c); git_SHA1_Update(&c, hdr, hdrlen); git_SHA1_Update(&c, dat->buf, dat->len); -- cgit v1.2.3 From c7ab0ba3405dc6bc8ade1296ef070a5a89660e76 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:08:12 -0400 Subject: avoid sprintf and strcpy with flex arrays When we are allocating a struct with a FLEX_ARRAY member, we generally compute the size of the array and then sprintf or strcpy into it. Normally we could improve a dynamic allocation like this by using xstrfmt, but it doesn't work here; we have to account for the size of the rest of the struct. But we can improve things a bit by storing the length that we use for the allocation, and then feeding it to xsnprintf or memcpy, which makes it more obvious that we are not writing more than the allocated number of bytes. It would be nice if we had some kind of helper for allocating generic flex arrays, but it doesn't work that well: - the call signature is a little bit unwieldy: d = flex_struct(sizeof(*d), offsetof(d, path), fmt, ...); You need offsetof here instead of just writing to the end of the base size, because we don't know how the struct is packed (partially this is because FLEX_ARRAY might not be zero, though we can account for that; but the size of the struct may actually be rounded up for alignment, and we can't know that). - some sites do clever things, like over-allocating because they know they will write larger things into the buffer later (e.g., struct packed_git here). So we're better off to just write out each allocation (or add type-specific helpers, though many of these are one-off allocations anyway). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'fast-import.c') diff --git a/fast-import.c b/fast-import.c index d0c25024cd..895c6b4a7e 100644 --- a/fast-import.c +++ b/fast-import.c @@ -863,13 +863,15 @@ static void start_packfile(void) { static char tmp_file[PATH_MAX]; struct packed_git *p; + int namelen; struct pack_header hdr; int pack_fd; pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_pack_XXXXXX"); - p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2); - strcpy(p->pack_name, tmp_file); + namelen = strlen(tmp_file) + 2; + p = xcalloc(1, sizeof(*p) + namelen); + xsnprintf(p->pack_name, namelen, "%s", tmp_file); p->pack_fd = pack_fd; p->do_not_close = 1; pack_file = sha1fd(pack_fd, p->pack_name); -- cgit v1.2.3 From 34fa79a6cde56d6d428ab0d3160cb094ebad3305 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:08:19 -0400 Subject: prefer memcpy to strcpy When we already know the length of a string (e.g., because we just malloc'd to fit it), it's nicer to use memcpy than strcpy, as it makes it more obvious that we are not going to overflow the buffer (because the size we pass matches the size in the allocation). This also eliminates calls to strcpy, which make auditing the code base harder. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'fast-import.c') diff --git a/fast-import.c b/fast-import.c index 895c6b4a7e..cf6d8bc0ce 100644 --- a/fast-import.c +++ b/fast-import.c @@ -644,8 +644,9 @@ static void *pool_calloc(size_t count, size_t size) static char *pool_strdup(const char *s) { - char *r = pool_alloc(strlen(s) + 1); - strcpy(r, s); + size_t len = strlen(s) + 1; + char *r = pool_alloc(len); + memcpy(r, s, len); return r; } -- cgit v1.2.3 From eddda371449ba925d91d04c615d084adf1b43a33 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:08:26 -0400 Subject: convert strncpy to memcpy strncpy is known to be a confusing function because of its termination semantics. These calls are all correct, but it takes some examination to see why. In particular, every one of them expects to copy up to the length limit, and then makes some arrangement for terminating the result. We can just use memcpy, along with noting explicitly how the result is terminated (if it is not already obvious). That should make it more clear to a reader that we are doing the right thing. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fast-import.c') diff --git a/fast-import.c b/fast-import.c index cf6d8bc0ce..4d01efcb9d 100644 --- a/fast-import.c +++ b/fast-import.c @@ -703,7 +703,7 @@ static struct atom_str *to_atom(const char *s, unsigned short len) c = pool_alloc(sizeof(struct atom_str) + len + 1); c->str_len = len; - strncpy(c->str_dat, s, len); + memcpy(c->str_dat, s, len); c->str_dat[len] = 0; c->next_atom = atom_table[hc]; atom_table[hc] = c; -- cgit v1.2.3