Some defensive programming #1890
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `lookup_commit_reference()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `parse_object()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `parse_tree_indirect()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `branch_get()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `branch_get()` can return NULL values. Signed-off-by: Johannes Schindelin <[email protected]>
CodeQL points out that `lookup_commit_reference()` can return NULL values. Signed-off-by: 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. Signed-off-by: Johannes Schindelin <[email protected]>
As pointed out by CodeQL, it could be NULL and we usually check for that. Signed-off-by: Johannes Schindelin <[email protected]>
On the off-chance that it's NULL... Signed-off-by: Johannes Schindelin <[email protected]>
As pointed out by CodeQL, `lookup_commit()` can return NULL. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
defensive-programming
branch
from
7187789 to
17e9e9a
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -3359,6 +3359,9 @@ static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *co | |||
| struct commit_list *p; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> 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.
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?
> Pointed out by CodeQL.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> revision.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index c4390f0938cb..59eae4eb8ba8 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3359,6 +3359,9 @@ static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *co
> struct 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) {
| @@ -1106,7 +1106,7 @@ static enum get_oid_result get_parent(struct repository *r, | |||
| if (ret) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
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(-)
>
> diff --git a/object-name.c b/object-name.c
> index 76749fbfe652..ca54dad2f4c8 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -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.
> if (!idx) {
> oidcpy(result, &commit->object.oid);
| @@ -155,7 +155,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid, | |||
| struct tag *tag = (struct tag *) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
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(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 1ed5e11dd568..4cbcb0c14c48 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -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.
> if (mark_tags_complete_and_check_obj_db)
> tag->object.flags |= COMPLETE;
| @@ -1106,7 +1106,7 @@ static enum get_oid_result get_parent(struct repository *r, | |||
| if (ret) | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Thu, May 15, 2025 at 12:45:27PM +0000, Johannes Schindelin via GitGitGadget wrote:
> CodeQL points out that `lookup_commit_reference()` can return NULL
> values.
> [...]
> commit = 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
|
User |
|
This patch series was integrated into seen via git@d36a872. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@20c7768. |
|
There was a status update in the "New Topics" section about the branch Assorted changes that please CodeQL. Comments? source: <[email protected]> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These patches implement some defensive programming to address complaints some static analyzers might have.
cc: Jeff King [email protected]