@matheusbsbs / matheus-bagliano / commits / c7ab0ba340

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 <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 24, 2015 at 17:08 UTC c7ab0ba3405dc6bc8ade1296ef070a5a89660e76

6 files changed +21 -14

archive.c

+3 -2

@@ -171,13 +171,14 @@ static void queue_directory(const unsigned char *sha1,

171 unsigned mode, int stage, struct archiver_context *c)

172 {

173 struct directory *d;

174 - d = xmallocz(sizeof(*d) + base->len + 1 + strlen(filename));

174 + size_t len = base->len + 1 + strlen(filename) + 1;

175 + d = xmalloc(sizeof(*d) + len);

176 d->up = c->bottom;

177 d->baselen = base->len;

178 d->mode = mode;

179 d->stage = stage;

180 c->bottom = d;

180 - d->len = sprintf(d->path, "%.*s%s/", (int)base->len, base->buf, filename);

181 + d->len = xsnprintf(d->path, len, "%.*s%s/", (int)base->len, base->buf, filename);

182 hashcpy(d->oid.hash, sha1);

183 }

184

builtin/blame.c

+3 -2

@@ -459,12 +459,13 @@ static void queue_blames(struct scoreboard *sb, struct origin *porigin,

459 static struct origin *make_origin(struct commit *commit, const char *path)

460 {

461 struct origin *o;

462 - o = xcalloc(1, sizeof(*o) + strlen(path) + 1);

462 + size_t pathlen = strlen(path) + 1;

463 + o = xcalloc(1, sizeof(*o) + pathlen);

464 o->commit = commit;

465 o->refcnt = 1;

466 o->next = commit->util;

467 commit->util = o;

467 - strcpy(o->path, path);

468 + memcpy(o->path, path, pathlen); /* includes NUL */

469 return o;

470 }

471

fast-import.c

+4 -2

@@ -863,13 +863,15 @@ static void start_packfile(void)

863 {

864 static char tmp_file[PATH_MAX];

865 struct packed_git *p;

866 + int namelen;

867 struct pack_header hdr;

868 int pack_fd;

869

870 pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),

871 "pack/tmp_pack_XXXXXX");

871 - p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2);

872 - strcpy(p->pack_name, tmp_file);

872 + namelen = strlen(tmp_file) + 2;

873 + p = xcalloc(1, sizeof(*p) + namelen);

874 + xsnprintf(p->pack_name, namelen, "%s", tmp_file);

875 p->pack_fd = pack_fd;

876 p->do_not_close = 1;

877 pack_file = sha1fd(pack_fd, p->pack_name);

refs.c

+4 -4

@@ -2695,7 +2695,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)

2695 int namelen = strlen(entry->name) + 1;

2696 struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);

2697 hashcpy(n->sha1, entry->u.value.oid.hash);

2698 - strcpy(n->name, entry->name);

2698 + memcpy(n->name, entry->name, namelen); /* includes NUL */

2699 n->next = cb->ref_to_prune;

2700 cb->ref_to_prune = n;

2701 }

@@ -3984,10 +3984,10 @@ void ref_transaction_free(struct ref_transaction *transaction)

3984 static struct ref_update *add_update(struct ref_transaction *transaction,

3985 const char *refname)

3986 {

3987 - size_t len = strlen(refname);

3988 - struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);

3987 + size_t len = strlen(refname) + 1;

3988 + struct ref_update *update = xcalloc(1, sizeof(*update) + len);

3989

3990 - strcpy((char *)update->refname, refname);

3990 + memcpy((char *)update->refname, refname, len); /* includes NUL */

3991 ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);

3992 transaction->updates[transaction->nr++] = update;

3993 return update;

sha1_file.c

+3 -2

@@ -1180,9 +1180,10 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)

1180 struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)

1181 {

1182 const char *path = sha1_pack_name(sha1);

1183 - struct packed_git *p = alloc_packed_git(strlen(path) + 1);

1183 + int alloc = strlen(path) + 1;

1184 + struct packed_git *p = alloc_packed_git(alloc);

1185

1185 - strcpy(p->pack_name, path);

1186 + memcpy(p->pack_name, path, alloc); /* includes NUL */

1187 hashcpy(p->sha1, sha1);

1188 if (check_packed_git_idx(idx_path, p)) {

1189 free(p);

submodule.c

+4 -2

@@ -122,6 +122,7 @@ static int add_submodule_odb(const char *path)

122 struct strbuf objects_directory = STRBUF_INIT;

123 struct alternate_object_database *alt_odb;

124 int ret = 0;

125 + int alloc;

126 const char *git_dir;

127

128 strbuf_addf(&objects_directory, "%s/.git", path);

@@ -142,9 +143,10 @@ static int add_submodule_odb(const char *path)

143 objects_directory.len))

144 goto done;

145

145 - alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));

146 + alloc = objects_directory.len + 42; /* for "12/345..." sha1 */

147 + alt_odb = xmalloc(sizeof(*alt_odb) + alloc);

148 alt_odb->next = alt_odb_list;

147 - strcpy(alt_odb->base, objects_directory.buf);

149 + xsnprintf(alt_odb->base, alloc, "%s", objects_directory.buf);

150 alt_odb->name = alt_odb->base + objects_directory.len;

151 alt_odb->name[2] = '/';

152 alt_odb->name[40] = '\0';