Some defensive programming
cover Some defensive programming
These patches implement some defensive programming to address complaints some static analyzers might have.
Johannes Schindelin (14):
revision: defensive programming
get_parent(): defensive programming
fetch-pack: defensive programming
unparse_commit(): defensive programming verify_commit_graph(): defensive programming
stash: defensive programming
stash: defensive programming
push: defensive programming
fetch: defensive programming
describe: defensive programming
inherit_tracking(): defensive programming
submodule: check return value of submodule_from_path() test-tool repository: check return value of lookup_commit()
shallow: handle missing shallow commits gracefully
branch.c | 2 ++ builtin/describe.c | 2 ++ builtin/fetch.c | 3 ++- builtin/push.c | 2 +- builtin/stash.c | 7 ++++++- builtin/submodule--helper.c | 3 +++ commit-graph.c | 5 +++++ commit.c | 2 +- fetch-pack.c | 2 +- object-name.c | 2 +- revision.c | 3 +++ shallow.c | 3 ++- t/helper/test-repository.c | 2 ++ 13 files changed, 31 insertions(+), 7 deletions(-)
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1890%2Fdscho%2Fdefensive-programming-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1890/dscho/defensive-programming-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1890
Signature · 1 line
gitgitgadget
PATCH 1/14 revision: defensive programming
From: Johannes Schindelin <[email protected]>
On the off chance that lookup_decoration() cannot find anything, let leave_one_treesame_to_parent() return gracefully instead of crashing.
Pointed out by CodeQL.
--- revision.c | 3 +++ 1 file changed, 3 insertions(+)
revision.c +3 -0
@@ -3359,6 +3359,9 @@ static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *costruct commit_list *p;unsigned n; +if (!ts) +return 0 ; +for (p = commit->parents, n =0 ; p; p = p->next, n++) {if (ts->treesame[n]) {if (p->item->object.flags &TMP_MARK ) {
Signature · 1 line
gitgitgadget
Re: [PATCH 01/14] revision: defensive programming
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
From: Johannes Schindelin <[email protected]>
On the off chance that
lookup_decoration()cannot find anything, letleave_one_treesame_to_parent()return gracefully instead of crashing.
But wouldn't it be a BUG("") worthy event for the treesame decoration not to exist for the commit object in question at this point of the code? Is it really defensive to silently pretend that nothing bad happened and to move forward?
Quoted 21 lines
Pointed out by CodeQL.
Signed-off-by Johannes Schindelin <[email protected]>--- revision.c | 3 +++ 1 file changed, 3 insertions(+)revision.c +3
-0@@ -3359,6 +3359,9 @@ static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *costruct commit_list *p;unsigned n; +if (!ts) +return 0 ; +for (p = commit->parents, n =0 ; p; p = p->next, n++) {if (ts->treesame[n]) {if (p->item->object.flags &TMP_MARK ) {
PATCH 2/14 get_parent(): defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that lookup_commit_reference() can return NULL values.
--- object-name.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
object-name.c +1 -1
@@ -1106,7 +1106,7 @@ static enum get_oid_result get_parent(struct repository *r,if (ret)return ret; commit =lookup_commit_reference (r, &oid); -if (repo_parse_commit (r, commit)) +if (!commit ||repo_parse_commit (r, commit))return MISSING_OBJECT ;if (!idx) {oidcpy (result, &commit->object.oid);
Signature · 1 line
gitgitgadget
Re: [PATCH 02/14] get_parent(): defensive programming
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 15 lines · writes:
From: Johannes Schindelin <[email protected]>
CodeQL points out that
lookup_commit_reference()can return NULL values.Signed-off-by Johannes Schindelin <[email protected]>--- object-name.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)object-name.c +0
-0
object-name.c static enum get_oid_result get_parent(struct repository *r, · line 1110
Quoted 6 lines
object-name.c +1
-1@@ -1106,7 +1106,7 @@ static enum get_oid_result get_parent(struct repository *r,if (ret)return ret; commit =lookup_commit_reference (r, &oid); -if (repo_parse_commit (r, commit)) +if (!commit ||repo_parse_commit (r, commit))return MISSING_OBJECT ;
Most of the time, the check for "ret" we see in the pre-context, which is a result of get_oid_1(), would prevent an oid that is not a valid name for a committish to even reach this code, I would think, but with possible repository corruption, we may fail to "lookup" the commit, so this is a good correction, I would think.
Thanks.
Quoted 2 lines
object-name.c +0
-0if (!idx) {oidcpy (result, &commit->object.oid);
Re: [PATCH 02/14] get_parent(): defensive programming
In reply to Johannes Schindelin via GitGitGadgetobject-name.c static enum get_oid_result get_parent(struct repository *r, · line 1110
Quoted 7 lines · Johannes Schindelin via GitGitGadget
CodeQL points out that
lookup_commit_reference()can return NULL values.
[...]object-name.c +1
-1commit =lookup_commit_reference (r, &oid); -if (repo_parse_commit (r, commit)) +if (!commit ||repo_parse_commit (r, commit))return MISSING_OBJECT ;
Sure, but repo_parse_commit() can also handle NULL values. It returns "-1" in that case. I think CodeQL is not smart enough to know that.
-Peff
PATCH 3/14 fetch-pack: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that parse_object() can return NULL values.
--- fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fetch-pack.c +1 -1
@@ -155,7 +155,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,struct tag *tag = (struct tag *)parse_object (the_repository, oid); -if (!tag->tagged) +if (!tag || !tag->tagged)return NULL ;if (mark_tags_complete_and_check_obj_db) tag->object.flags |=COMPLETE ;
Signature · 1 line
gitgitgadget
Re: [PATCH 03/14] fetch-pack: defensive programming
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 14 lines · writes:
From: Johannes Schindelin <[email protected]>
CodeQL points out that
parse_object()can return NULL values.Signed-off-by Johannes Schindelin <[email protected]>--- fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fetch-pack.c +0
-0
fetch-pack.c static struct commit *deref_without_lazy_fetch(const struct object_id *oid, · line 159
Quoted 6 lines
fetch-pack.c +1
-1@@ -155,7 +155,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,struct tag *tag = (struct tag *)parse_object (the_repository, oid); -if (!tag->tagged) +if (!tag || !tag->tagged)return NULL ;
The "oid" can come from corruptible sources like commit graph file, so I agree with your analysis that it may name a missing object in a corrupt repository, leading "tag" being NULL. Looks correct.
Quoted 2 lines
fetch-pack.c +0
-0if (mark_tags_complete_and_check_obj_db) tag->object.flags |=COMPLETE ;
PATCH 4/14 unparse_commit(): defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that lookup_commit() can return NULL values.
--- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
commit.c +1 -1
@@ -187,7 +187,7 @@ void unparse_commit(struct repository *r, const struct object_id *oid) {struct commit *c =lookup_commit (r, oid); -if (!c->object.parsed) +if (!c || !c->object.parsed)return ;free_commit_list (c->parents); c->parents =NULL ;
Signature · 1 line
gitgitgadget
PATCH 5/14 verify_commit_graph(): defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that lookup_commit() can return NULL values.
--- commit-graph.c | 5 +++++ 1 file changed, 5 insertions(+)
commit-graph.c +5 -0
@@ -2786,6 +2786,11 @@ static int verify_one_commit_graph(struct repository *r, the_repository->hash_algo); graph_commit =lookup_commit (r, &cur_oid); +if (!graph_commit) { +graph_report (_ ("failed to look up commit %s for commit-graph" ), +oid_to_hex (&cur_oid)); +continue ; + } odb_commit = (struct commit *)create_object (r, &cur_oid,alloc_commit_node (r));if (repo_parse_commit_internal (r, odb_commit,0 ,0 )) { graph_report(_("failed to parse commit %s from object database for commit-graph"),
Signature · 1 line
gitgitgadget
PATCH 6/14 stash: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that parse_tree_indirect() can return NULL values.
--- builtin/stash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/stash.c +1 -1
@@ -285,7 +285,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)memset (&opts,0 ,sizeof (opts)); tree =parse_tree_indirect (i_tree); -if (parse_tree (tree)) +if (!tree ||parse_tree (tree))return -1 ;init_tree_desc (t, &tree->object.oid, tree->buffer, tree->size);
Signature · 1 line
gitgitgadget
PATCH 7/14 stash: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that lookup_commit() can return NULL values.
--- builtin/stash.c | 5 +++++ 1 file changed, 5 insertions(+)
builtin/stash.c +5 -0
@@ -1396,6 +1396,11 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b goto done; }else { head_commit =lookup_commit (the_repository, &info->b_commit); +if (!head_commit) { + ret =error (_ ("could not look up commit '%s'" ), +oid_to_hex (&info->b_commit)); + goto done; + } }if (!check_changes (ps, include_untracked, &untracked_files)) {
Signature · 1 line
gitgitgadget
PATCH 8/14 push: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that branch_get() can return NULL values.
--- builtin/push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/push.c +1 -1
@@ -90,7 +90,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,if (push_default ==PUSH_DEFAULT_UPSTREAM &&skip_prefix (matched->name,"refs/heads/" , &branch_name)) {struct branch *branch =branch_get (branch_name); -if (branch->merge_nr ==1 && branch->merge[0 ]->src) { +if (branch && branch->merge_nr ==1 && branch->merge[0 ]->src) {refspec_appendf (refspec,"%s:%s" , ref, branch->merge[0 ]->src);return ;
Signature · 1 line
gitgitgadget
PATCH 9/14 fetch: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that branch_get() can return NULL values.
--- builtin/fetch.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/fetch.c +2 -1
@@ -553,7 +553,7 @@ static struct ref *get_ref_map(struct remote *remote,if (remote && (remote->fetch.nr ||/* Note: has_merge implies non-NULL branch->remote_name */ - (has_merge && !strcmp (branch->remote_name, remote->name)))) { + (has_merge && branch && !strcmp (branch->remote_name, remote->name)))) {for (i =0 ; i < remote->fetch.nr; i++) {get_fetch_map (remote_refs, &remote->fetch.items[i], &tail,0 ); if (remote->fetch.items[i].dst &&
@@ -571,6 +571,7 @@ static struct ref *get_ref_map(struct remote *remote,* Note: has_merge implies non-NULL branch->remote_name */ if (has_merge && + branch && !strcmp (branch->remote_name, remote->name))add_merge_config (&ref_map, remote_refs, branch, &tail); }else if (!prefetch) {
Signature · 1 line
gitgitgadget
PATCH 10/14 describe: defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that lookup_commit_reference() can return NULL values.
--- builtin/describe.c | 2 ++ 1 file changed, 2 insertions(+)
builtin/describe.c +2 -0
@@ -324,6 +324,8 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)unsigned int unannotated_cnt =0 ; cmit =lookup_commit_reference (the_repository, oid); +if (!cmit) +die (_ ("could not look up commit '%s'" ),oid_to_hex (oid)); n =find_commit_name (&cmit->object.oid);if (n && (tags || all || n->prio ==2 )) {
Signature · 1 line
gitgitgadget
PATCH 11/14 inherit_tracking(): defensive programming
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
CodeQL points out that branch_get() can return NULL values.
Note that the error path in this instance calls BUG(), not die(), for two reasons:
1. The code lives in libgit.a and calling die() from within those library functions is a bad practice that needs to be reduced, rather than increased.
2. The inherit_tracking() function really should only be called with the name of an existing branch, therefore a NULL return value would indeed constitute a bug in Git's code.
--- branch.c | 2 ++ 1 file changed, 2 insertions(+)
branch.c +2 -0
@@ -224,6 +224,8 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)skip_prefix (orig_ref,"refs/heads/" , &bare_ref); branch =branch_get (bare_ref); +if (!branch) +BUG ("could not get branch for '%s" , bare_ref);if (!branch->remote_name) {warning (_ ("asked to inherit tracking from '%s', but no remote is set" ), bare_ref);
Signature · 1 line
gitgitgadget
PATCH 12/14 submodule: check return value of `submodule_from_path()`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
As pointed out by CodeQL, it could be NULL and we usually check for that.
--- builtin/submodule--helper.c | 3 +++ 1 file changed, 3 insertions(+)
builtin/submodule--helper.c +3 -0
@@ -1934,6 +1934,9 @@ static int determine_submodule_update_strategy(struct repository *r,const char *val;int ret; +if (!sub) +return error (_ ("could not retrieve submodule information for path '%s'" ), path); + key =xstrfmt ("submodule.%s.update" , sub->name);if (update) {
Signature · 1 line
gitgitgadget
PATCH 13/14 test-tool repository: check return value of `lookup_commit()`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
On the off-chance that it's NULL...
--- t/helper/test-repository.c | 2 ++ 1 file changed, 2 insertions(+)
t/helper/test-repository.c +2 -0
@@ -27,6 +27,8 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,repo_set_hash_algo (the_repository,hash_algo_by_ptr (r.hash_algo)); c =lookup_commit (&r, commit_oid); +if (!c) +die ("Could not look up %s" ,oid_to_hex (commit_oid));if (!parse_commit_in_graph (&r, c))die ("Couldn't parse commit" );
Signature · 1 line
gitgitgadget
PATCH 14/14 shallow: handle missing shallow commits gracefully
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
As pointed out by CodeQL, lookup_commit() can return NULL.
--- shallow.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
shallow.c +2 -1
@@ -702,7 +702,8 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,for (i =0 ; i < nr_shallow; i++) {struct commit *c =lookup_commit (the_repository, &oid[shallow[i]]); - c->object.flags |=BOTTOM ; +if (c) + c->object.flags |=BOTTOM ; }for (i =0 ; i < ref->nr; i++)
Signature · 1 line
gitgitgadget