From 4f9327faf9aa5db034bf45d3aa9c23987742acb0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 27 Mar 2017 13:11:38 +0200 Subject: [PATCH 1/3] treebuilder: fix memory leaks in `write_with_buffer` While we detect errors in `git_treebuilder_write_with_buffer`, we just exit directly instead of freeing allocated memory. Fix this by remembering error codes and skipping forward to the function's cleanup code. --- src/tree.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/tree.c b/src/tree.c index dba2060c3..d781fdebc 100644 --- a/src/tree.c +++ b/src/tree.c @@ -818,7 +818,7 @@ int git_treebuilder_write_with_buffer(git_oid *oid, git_treebuilder *bld, git_bu size_t i, entrycount; git_odb *odb; git_tree_entry *entry; - git_vector entries; + git_vector entries = GIT_VECTOR_INIT; assert(bld); assert(tree); @@ -826,16 +826,16 @@ int git_treebuilder_write_with_buffer(git_oid *oid, git_treebuilder *bld, git_bu git_buf_clear(tree); entrycount = git_strmap_num_entries(bld->map); - if (git_vector_init(&entries, entrycount, entry_sort_cmp) < 0) - return -1; + if ((error = git_vector_init(&entries, entrycount, entry_sort_cmp)) < 0) + goto out; if (tree->asize == 0 && - (error = git_buf_grow(tree, entrycount * 72)) < 0) - return error; + (error = git_buf_grow(tree, entrycount * 72)) < 0) + goto out; git_strmap_foreach_value(bld->map, entry, { - if (git_vector_insert(&entries, entry) < 0) - return -1; + if ((error = git_vector_insert(&entries, entry)) < 0) + goto out; }); git_vector_sort(&entries); @@ -855,6 +855,7 @@ int git_treebuilder_write_with_buffer(git_oid *oid, git_treebuilder *bld, git_bu !(error = git_repository_odb__weakptr(&odb, bld->repo))) error = git_odb_write(oid, odb, tree->ptr, tree->size, GIT_OBJ_TREE); +out: git_vector_free(&entries); return error; From 8d1e71f5a24106bfeb4d6ff767708dd40e4a58dd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 27 Mar 2017 13:14:05 +0200 Subject: [PATCH 2/3] treebuilder: remove shadowing variable in `write_with_buffer` The `git_tree_entry *entry` variable is defined twice inside of this function. While this is not a problem currently, remove the shadowing variable to avoid future confusion. --- src/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tree.c b/src/tree.c index d781fdebc..f73701f0d 100644 --- a/src/tree.c +++ b/src/tree.c @@ -841,7 +841,7 @@ int git_treebuilder_write_with_buffer(git_oid *oid, git_treebuilder *bld, git_bu git_vector_sort(&entries); for (i = 0; i < entries.length && !error; ++i) { - git_tree_entry *entry = git_vector_get(&entries, i); + entry = git_vector_get(&entries, i); git_buf_printf(tree, "%o ", entry->attr); git_buf_put(tree, entry->filename, entry->filename_len + 1); From 06abbb7f07c64e877ffa87ae4614ae5f4e933435 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 27 Mar 2017 13:14:48 +0200 Subject: [PATCH 3/3] treebuilder: exit early if running OOM in `write_with_buffer` While writing the tree inside of a buffer, we check whether the buffer runs out of memory after each tree entry. While we set the error code as soon as we detect the OOM situation, we happily proceed iterating over the entries. This is not useful at all, as we will try to write into the buffer repeatedly, which cannot work. Fix this by exiting as soon as we are OOM. --- src/tree.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/tree.c b/src/tree.c index f73701f0d..6b1d1b238 100644 --- a/src/tree.c +++ b/src/tree.c @@ -847,12 +847,13 @@ int git_treebuilder_write_with_buffer(git_oid *oid, git_treebuilder *bld, git_bu git_buf_put(tree, entry->filename, entry->filename_len + 1); git_buf_put(tree, (char *)entry->oid->id, GIT_OID_RAWSZ); - if (git_buf_oom(tree)) + if (git_buf_oom(tree)) { error = -1; + goto out; + } } - if (!error && - !(error = git_repository_odb__weakptr(&odb, bld->repo))) + if ((error = git_repository_odb__weakptr(&odb, bld->repo)) == 0) error = git_odb_write(oid, odb, tree->ptr, tree->size, GIT_OBJ_TREE); out: