Re: [PATCH 08/18] use st_add and st_mult for allocation size computation

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:53 PM, Jeff King  wrote:
> If our size computation overflows size_t, we may allocate a
> much smaller buffer than we expected and overflow it. It's
> probably impossible to trigger an overflow in most of these
> sites in practice, but it is easy enough convert their
> additions and multiplications into overflow-checking
> variants. This may be fixing real bugs, and it makes
> auditing the code easier.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -2632,7 +2632,7 @@ static void update_image(struct image *img,
> -   result = xmalloc(img->len + insert_count - remove_count + 1);
> +   result = xmalloc(st_add3(st_sub(img->len, remove_count), 
> insert_count, 1));

Phew, what a mouthful, and not easy to read compared to the original.
Fortunately, the remainder of the changes in this patch are
straightforward and often simple.

> diff --git a/sha1_name.c b/sha1_name.c
> @@ -87,9 +87,8 @@ static void find_short_object_filename(int len, const char 
> *hex_pfx, struct disa
> const char *objdir = get_object_directory();
> -   int objdir_len = strlen(objdir);
> -   int entlen = objdir_len + 43;
> -   fakeent = xmalloc(sizeof(*fakeent) + entlen);
> +   size_t objdir_len = strlen(objdir);
> +   fakeent = xmalloc(st_add3(sizeof(*fakeent), objdir_len, 43));
> memcpy(fakeent->base, objdir, objdir_len);
> fakeent->name = fakeent->base + objdir_len + 1;

If we've gotten this far without die()ing due to overflow in st_add3()
when invoking xmalloc(), then we know that this fakeent->name
computation won't overflow. Okay.

> fakeent->name[-1] = '/';
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/18] use st_add and st_mult for allocation size computation

[Jeff King]

If our size computation overflows size_t, we may allocate a
much smaller buffer than we expected and overflow it. It's
probably impossible to trigger an overflow in most of these
sites in practice, but it is easy enough convert their
additions and multiplications into overflow-checking
variants. This may be fixing real bugs, and it makes
auditing the code easier.

Signed-off-by: Jeff King 
---
 archive.c  |  4 ++--
 builtin/apply.c|  2 +-
 builtin/clean.c|  2 +-
 builtin/fetch.c|  2 +-
 builtin/index-pack.c   |  4 ++--
 builtin/merge.c|  2 +-
 builtin/mv.c   |  4 ++--
 builtin/receive-pack.c |  2 +-
 combine-diff.c | 14 +++---
 commit.c   |  2 +-
 compat/mingw.c |  4 ++--
 compat/qsort.c |  2 +-
 compat/setenv.c|  2 +-
 compat/win32/syslog.c  |  4 ++--
 diffcore-delta.c   |  6 --
 diffcore-rename.c  |  2 +-
 dir.c  |  4 ++--
 fast-import.c  |  2 +-
 refs.c |  2 +-
 remote.c   |  8 
 revision.c |  2 +-
 sha1_file.c| 20 +++-
 sha1_name.c|  5 ++---
 shallow.c  |  2 +-
 submodule.c|  6 +++---
 25 files changed, 56 insertions(+), 53 deletions(-)

diff --git a/archive.c b/archive.c
index 0687afa..5d735ae 100644
--- a/archive.c
+++ b/archive.c
@@ -171,8 +171,8 @@ static void queue_directory(const unsigned char *sha1,
unsigned mode, int stage, struct archiver_context *c)
 {
struct directory *d;
-   size_t len = base->len + 1 + strlen(filename) + 1;
-   d = xmalloc(sizeof(*d) + len);
+   size_t len = st_add4(base->len, 1, strlen(filename), 1);
+   d = xmalloc(st_add(sizeof(*d), len));
d->up  = c->bottom;
d->baselen = base->len;
d->mode= mode;
diff --git a/builtin/apply.c b/builtin/apply.c
index d61ac65..42c610e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2632,7 +2632,7 @@ static void update_image(struct image *img,
insert_count = postimage->len;
 
/* Adjust the contents */
-   result = xmalloc(img->len + insert_count - remove_count + 1);
+   result = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 
1));
memcpy(result, img->buf, applied_at);
memcpy(result + applied_at, postimage->buf, postimage->len);
memcpy(result + applied_at + postimage->len,
diff --git a/builtin/clean.c b/builtin/clean.c
index 8229f7e..0371010 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -615,7 +615,7 @@ static int *list_and_choose(struct menu_opts *opts, struct 
menu_stuff *stuff)
nr += chosen[i];
}
 
-   result = xcalloc(nr + 1, sizeof(int));
+   result = xcalloc(st_add(nr, 1), sizeof(int));
for (i = 0; i < stuff->nr && j < nr; i++) {
if (chosen[i])
result[j++] = i;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..373a89d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1110,7 +1110,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
if (argc > 0) {
int j = 0;
int i;
-   refs = xcalloc(argc + 1, sizeof(const char *));
+   refs = xcalloc(st_add(argc, 1), sizeof(const char *));
for (i = 0; i < argc; i++) {
if (!strcmp(argv[i], "tag")) {
i++;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a60bcfa..193908a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1744,9 +1744,9 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
 
curr_pack = open_pack_file(pack_name);
parse_pack_header();
-   objects = xcalloc(nr_objects + 1, sizeof(struct object_entry));
+   objects = xcalloc(st_add(nr_objects, 1), sizeof(struct object_entry));
if (show_stat)
-   obj_stat = xcalloc(nr_objects + 1, sizeof(struct object_stat));
+   obj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct 
object_stat));
ofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));
parse_pack_objects(pack_sha1);
resolve_deltas();
diff --git a/builtin/merge.c b/builtin/merge.c
index b98a348..101ffef 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -939,7 +939,7 @@ static int setup_with_upstream(const char ***argv)
if (!branch->merge_nr)
die(_("No default upstream defined for the current branch."));
 
-   args = xcalloc(branch->merge_nr + 1, sizeof(char *));
+   args = xcalloc(st_add(branch->merge_nr, 1), sizeof(char *));
for (i = 0; i < branch->merge_nr; i++) {
if (!branch->merge[i]->dst)
die(_("No remote-tracking branch for %s from %s"),
diff --git a/builti