From b27ccad274a861415517f5b35800a45a32535e14 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 16:11:51 +0100 Subject: [PATCH 1/6] refdb_fs: fail if refcache returns NULL pointer We usually check entries returned by `git_sortedcache_entry` for NULL pointers. As we have a write lock in `packed_write`, though, it really should not happen that the function returns NULL. Assert that ref is not NULL to silence a Coverity warning. --- src/refdb_fs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index f6ed7201a..f978038e6 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -962,6 +962,7 @@ static int packed_write(refdb_fs_backend *backend) for (i = 0; i < git_sortedcache_entrycount(refcache); ++i) { struct packref *ref = git_sortedcache_entry(refcache, i); + assert(ref); if (packed_find_peel(backend, ref) < 0) goto fail; From 8a4a343a2b230acc69ba13131355f8299b4483d3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 16:33:49 +0100 Subject: [PATCH 2/6] blame_git: handle error returned by `git_commit_parent` --- src/blame_git.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/blame_git.c b/src/blame_git.c index b8b568285..700207edb 100644 --- a/src/blame_git.c +++ b/src/blame_git.c @@ -525,7 +525,8 @@ static int pass_blame(git_blame *blame, git_blame__origin *origin, uint32_t opt) if (sg_origin[i]) continue; - git_commit_parent(&p, origin->commit, i); + if ((error = git_commit_parent(&p, origin->commit, i)) < 0) + goto finish; porigin = find_origin(blame, p, origin); if (!porigin) From e850e98ddbcca9d51f412482e3bf16b2d5c36bc8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 16:42:55 +0100 Subject: [PATCH 3/6] blame: handle error when resoling HEAD in normalize_options When normalizing options we try to look up HEAD's OID. While this action may fail in malformed repositories we never check the return value of the function. Fix the issue by converting `normalize_options` to actually return an error and handle the error in `git_blame_file`. --- src/blame.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/blame.c b/src/blame.c index 2daf91591..2c8584ba5 100644 --- a/src/blame.c +++ b/src/blame.c @@ -178,7 +178,7 @@ const git_blame_hunk *git_blame_get_hunk_byline(git_blame *blame, size_t lineno) return NULL; } -static void normalize_options( +static int normalize_options( git_blame_options *out, const git_blame_options *in, git_repository *repo) @@ -190,7 +190,9 @@ static void normalize_options( /* No newest_commit => HEAD */ if (git_oid_iszero(&out->newest_commit)) { - git_reference_name_to_id(&out->newest_commit, repo, "HEAD"); + if (git_reference_name_to_id(&out->newest_commit, repo, "HEAD") < 0) { + return -1; + } } /* min_line 0 really means 1 */ @@ -204,6 +206,8 @@ static void normalize_options( out->flags |= GIT_BLAME_TRACK_COPIES_SAME_COMMIT_MOVES; if (out->flags & GIT_BLAME_TRACK_COPIES_SAME_COMMIT_MOVES) out->flags |= GIT_BLAME_TRACK_COPIES_SAME_FILE; + + return 0; } static git_blame_hunk *split_hunk_in_vector( @@ -362,7 +366,8 @@ int git_blame_file( git_blame *blame = NULL; assert(out && repo && path); - normalize_options(&normOptions, options, repo); + if ((error = normalize_options(&normOptions, options, repo)) < 0) + goto on_error; blame = git_blame__alloc(repo, normOptions, path); GITERR_CHECK_ALLOC(blame); From 836447e586f275a1f0b32860805f69ad6867ec32 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 16:52:09 +0100 Subject: [PATCH 4/6] config_file: handle error when trying to lock strmap Accessing the current values map is handled through the `refcounder_strmap_take` function, which first acquires a mutex before accessing its values. While this assures everybody is trying to access the values with the mutex only we do not check if the locking actually succeeds. Fix the issue by checking if acquiring the lock succeeds and returning `NULL` if we encounter an error. Adjust callers. --- src/config_file.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 5f5e309e0..d79175af9 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -232,7 +232,10 @@ static refcounted_strmap *refcounted_strmap_take(diskfile_header *h) { refcounted_strmap *map; - git_mutex_lock(&h->values_mutex); + if (git_mutex_lock(&h->values_mutex) < 0) { + giterr_set(GITERR_OS, "Failed to lock config backend"); + return NULL; + } map = h->values; git_atomic_inc(&map->refcount); @@ -318,7 +321,10 @@ static int config__refresh(git_config_backend *cfg) if ((error = config_read(values->values, b, reader, b->level, 0)) < 0) goto out; - git_mutex_lock(&b->header.values_mutex); + if ((error = git_mutex_lock(&b->header.values_mutex)) < 0) { + giterr_set(GITERR_OS, "Failed to lock config backend"); + goto out; + } tmp = b->header.values; b->header.values = values; @@ -460,7 +466,8 @@ static int config_set(git_config_backend *cfg, const char *name, const char *val if ((rval = git_config__normalize_name(name, &key)) < 0) return rval; - map = refcounted_strmap_take(&b->header); + if ((map = refcounted_strmap_take(&b->header)) == NULL) + return -1; values = map->values; /* @@ -527,7 +534,8 @@ static int config_get(git_config_backend *cfg, const char *key, git_config_entry if (!h->parent.readonly && ((error = config_refresh(cfg)) < 0)) return error; - map = refcounted_strmap_take(h); + if ((map = refcounted_strmap_take(h)) == NULL) + return -1; values = map->values; pos = git_strmap_lookup_index(values, key); @@ -565,7 +573,8 @@ static int config_set_multivar( if ((result = git_config__normalize_name(name, &key)) < 0) return result; - map = refcounted_strmap_take(&b->header); + if ((map = refcounted_strmap_take(&b->header)) == NULL) + return -1; values = b->header.values->values; pos = git_strmap_lookup_index(values, key); @@ -610,7 +619,8 @@ static int config_delete(git_config_backend *cfg, const char *name) if ((result = git_config__normalize_name(name, &key)) < 0) return result; - map = refcounted_strmap_take(&b->header); + if ((map = refcounted_strmap_take(&b->header)) == NULL) + return -1; values = b->header.values->values; pos = git_strmap_lookup_index(values, key); @@ -649,7 +659,8 @@ static int config_delete_multivar(git_config_backend *cfg, const char *name, con if ((result = git_config__normalize_name(name, &key)) < 0) return result; - map = refcounted_strmap_take(&b->header); + if ((map = refcounted_strmap_take(&b->header)) == NULL) + return -1; values = b->header.values->values; pos = git_strmap_lookup_index(values, key); @@ -832,7 +843,8 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve /* We're just copying data, don't care about the level */ GIT_UNUSED(level); - src_map = refcounted_strmap_take(src_header); + if ((src_map = refcounted_strmap_take(src_header)) == NULL) + return -1; b->header.values = src_map; return 0; From 6ff8a7c4bed65f8fd7d7fbda662583499e68e2aa Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 17:05:30 +0100 Subject: [PATCH 5/6] filebuf: handle write error in `lock_file` When writing to a file with locking not check if writing the locked file actually succeeds. Fix the issue by returning error code and message when writing fails. --- src/filebuf.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/filebuf.c b/src/filebuf.c index 17efe872e..101d5082a 100644 --- a/src/filebuf.c +++ b/src/filebuf.c @@ -70,6 +70,7 @@ static int lock_file(git_filebuf *file, int flags, mode_t mode) git_file source; char buffer[FILEIO_BUFSIZE]; ssize_t read_bytes; + int error; source = p_open(file->path_original, O_RDONLY); if (source < 0) { @@ -80,7 +81,8 @@ static int lock_file(git_filebuf *file, int flags, mode_t mode) } while ((read_bytes = p_read(source, buffer, sizeof(buffer))) > 0) { - p_write(file->fd, buffer, read_bytes); + if ((error = p_write(file->fd, buffer, read_bytes)) < 0) + break; if (file->compute_digest) git_hash_update(&file->digest, buffer, read_bytes); } @@ -90,6 +92,9 @@ static int lock_file(git_filebuf *file, int flags, mode_t mode) if (read_bytes < 0) { giterr_set(GITERR_OS, "Failed to read file '%s'", file->path_original); return -1; + } else if (error < 0) { + giterr_set(GITERR_OS, "Failed to write file '%s'", file->path_lock); + return -1; } } From 13c371dc10f8b5696b5728c9824d155ef9ad2f81 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 17:21:02 +0100 Subject: [PATCH 6/6] config_cache: check return value of `git_config__lookup_entry` Callers of `git_config__cvar` already handle the case where the function returns an error due to a failed configuration variable lookup, but we are actually swallowing errors when calling `git_config__lookup_entry` inside of the function. Fix this by returning early when `git_config__lookup_entry` returns an error. As we call `git_config__lookup_entry` with `no_errors == false` which leads us to call `get_entry` with `GET_NO_MISSING` we will not return early when the lookup fails due to a missing entry. Like this we are still able to set the default value of the cvar and exit successfully. --- src/config_cache.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/config_cache.c b/src/config_cache.c index c859ec148..dbea871b9 100644 --- a/src/config_cache.c +++ b/src/config_cache.c @@ -86,7 +86,8 @@ int git_config__cvar(int *out, git_config *config, git_cvar_cached cvar) struct map_data *data = &_cvar_maps[(int)cvar]; git_config_entry *entry; - git_config__lookup_entry(&entry, config, data->cvar_name, false); + if ((error = git_config__lookup_entry(&entry, config, data->cvar_name, false)) < 0) + return error; if (!entry) *out = data->default_value;