From f80852af807ae40bfc56e3285dda460873ecc9f9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 2 May 2016 14:30:14 +0200 Subject: index: fix memory leak on error case --- src/index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.c b/src/index.c index 63e47965a..31cb27d6c 100644 --- a/src/index.c +++ b/src/index.c @@ -3008,7 +3008,7 @@ int git_index_read_index( if (error < 0) { giterr_set(GITERR_INDEX, "failed to insert entry"); - return error; + goto done; } if (diff <= 0) { -- cgit v1.2.1 From fcd1b786017471a7cbf97b1065a00f0551d47115 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 2 May 2016 14:46:14 +0200 Subject: merge_file: do not unnecessarily check ours/theirs for NULL The `merge_file__xdiff` function checks if either `ours` or `theirs` is `NULL`. The function is to be called with existing files, though, and in fact already unconditionally dereferences both pointers. Remove the unnecessary check to silence warnings. --- src/merge_file.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/merge_file.c b/src/merge_file.c index 731f4b724..3f14a4f63 100644 --- a/src/merge_file.c +++ b/src/merge_file.c @@ -134,8 +134,8 @@ static int merge_file__xdiff( path = git_merge_file__best_path( ancestor ? ancestor->path : NULL, - ours ? ours->path : NULL, - theirs ? theirs->path : NULL); + ours->path, + theirs->path); if (path != NULL && (out->path = git__strdup(path)) == NULL) { error = -1; @@ -147,8 +147,8 @@ static int merge_file__xdiff( out->len = mmbuffer.size; out->mode = git_merge_file__best_mode( ancestor ? ancestor->mode : 0, - ours ? ours->mode : 0, - theirs ? theirs->mode : 0); + ours->mode, + theirs->mode); done: if (error < 0) -- cgit v1.2.1 From 7b24c4fd48abc67792f2af82c0eb374618475d17 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 2 May 2016 15:47:54 +0200 Subject: checkout: set ignorecase=0 when config lookup fails When `git_repository__cvar` fails we may end up with a `ignorecase` value of `-1`. As we subsequently check if `ignorecase` is non-zero, we may end up reporting that data should be removed when in fact it should not. Err on the safer side and set `ignorecase = 0` when `git_repository__cvar` fails. --- src/checkout.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/checkout.c b/src/checkout.c index fed1819aa..d84b46ba7 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -1360,9 +1360,11 @@ fail: static bool should_remove_existing(checkout_data *data) { - int ignorecase = 0; + int ignorecase; - git_repository__cvar(&ignorecase, data->repo, GIT_CVAR_IGNORECASE); + if (git_repository__cvar(&ignorecase, data->repo, GIT_CVAR_IGNORECASE) < 0) { + ignorecase = 0; + } return (ignorecase && (data->strategy & GIT_CHECKOUT_DONT_REMOVE_EXISTING) == 0); -- cgit v1.2.1 From 7f407710ef5d46b18ee68f7f6580e593ee486bb4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 2 May 2016 16:24:14 +0200 Subject: odb_loose: fix undefined behavior when computing size An object's size is computed by reading the object header's size field until the most significant bit is not set anymore. To get the total size, we increase the shift on each iteration and add the shifted value to the total size. We read the current value into a variable of type `unsigned char`, from which we then take all bits except the most significant bit and shift the result. We will end up with a maximum shift of 60, but this exceeds the width of the value's type, resulting in undefined behavior. Fix the issue by instead reading the values into a variable of type `unsigned long`, which matches the required width. This is equivalent to git.git, which uses an `unsigned long` as well. --- src/odb_loose.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/odb_loose.c b/src/odb_loose.c index 9d9bffd21..3c33160d0 100644 --- a/src/odb_loose.c +++ b/src/odb_loose.c @@ -91,7 +91,7 @@ static int object_mkdir(const git_buf *name, const loose_backend *be) static size_t get_binary_object_header(obj_hdr *hdr, git_buf *obj) { - unsigned char c; + unsigned long c; unsigned char *data = (unsigned char *)obj->ptr; size_t shift, size, used = 0; -- cgit v1.2.1 From 153fde5b4349f12de4f152c26d3b298e58a69cd8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 2 May 2016 16:49:59 +0200 Subject: delta-apply: fix sign extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We compute offsets by executing `off |= (*delta++ << 24)` for multiple constants, where `off` is of type `size_t` and `delta` is of type `unsigned char`. The usual arithmetic conversions (see ISO C89 ยง3.2.1.5 "Usual arithmetic conversions") kick in here, causing us to promote both operands to `int` and then extending the result to an `unsigned long` when OR'ing it with `off`. The integer promotion to `int` may result in wrong size calculations for big values. Fix the issue by making the constants `unsigned long`, causing both operands to be promoted to `unsigned long`. --- src/delta-apply.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/delta-apply.c b/src/delta-apply.c index 89745faa0..3830c1c77 100644 --- a/src/delta-apply.c +++ b/src/delta-apply.c @@ -90,13 +90,13 @@ int git__delta_apply( size_t off = 0, len = 0; if (cmd & 0x01) off = *delta++; - if (cmd & 0x02) off |= *delta++ << 8; - if (cmd & 0x04) off |= *delta++ << 16; - if (cmd & 0x08) off |= *delta++ << 24; + if (cmd & 0x02) off |= *delta++ << 8UL; + if (cmd & 0x04) off |= *delta++ << 16UL; + if (cmd & 0x08) off |= *delta++ << 24UL; if (cmd & 0x10) len = *delta++; - if (cmd & 0x20) len |= *delta++ << 8; - if (cmd & 0x40) len |= *delta++ << 16; + if (cmd & 0x20) len |= *delta++ << 8UL; + if (cmd & 0x40) len |= *delta++ << 16UL; if (!len) len = 0x10000; if (base_len < off + len || res_sz < len) -- cgit v1.2.1 From fe3057b4b9bbab028d7cd18ce06edc034847f844 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 3 May 2016 17:36:09 +0200 Subject: diff: simplify code for handling empty dirs When determining diffs between two iterators we may need to recurse into an unmatched directory for the "new" iterator when it is either a prefix to the current item of the "old" iterator or when untracked/ignored changes are requested by the user and the directory is untracked/ignored. When advancing into the directory and no files are found, we will get back `GIT_ENOTFOUND`. If so, we simply skip the directory, handling resulting unmatched old items in the next iteration. The other case of `iterator_advance_into` returning either `GIT_NOERROR` or any other error but `GIT_ENOTFOUND` will be handled by the caller, which will now either compare the first directory entry of the "new" iterator in case of `GIT_ENOERROR` or abort on other cases. Improve readability of the code to make the above logic more clear. --- src/diff.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/diff.c b/src/diff.c index 64641daab..26c0b895b 100644 --- a/src/diff.c +++ b/src/diff.c @@ -1083,17 +1083,13 @@ static int handle_unmatched_new_item( if (recurse_into_dir) { error = iterator_advance_into(&info->nitem, info->new_iter); - /* if real error or no error, proceed with iteration */ - if (error != GIT_ENOTFOUND) - return error; - giterr_clear(); + /* if directory is empty, can't advance into it, so skip it */ + if (error == GIT_ENOTFOUND) { + giterr_clear(); + error = iterator_advance(&info->nitem, info->new_iter); + } - /* if directory is empty, can't advance into it, so either skip - * it or ignore it - */ - if (error == GIT_ENOTFOUND || contains_oitem) - return iterator_advance(&info->nitem, info->new_iter); - delta_type = GIT_DELTA_IGNORED; + return error; } } -- cgit v1.2.1