-
Notifications
You must be signed in to change notification settings - Fork 189
coverity: avoid dereferencing NULL #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
df00334
4fdba05
1398a2f
285f019
12c2c84
ca818ee
8216769
41285dd
cccd361
376a658
e581bc9
2ef74b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms) | |
|
|
||
| refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid); | ||
| commit = lookup_commit_reference_by_name(bad_ref); | ||
| if (!commit) { | ||
| error(_("could not find commit for '%s'"), bad_ref); | ||
| free(bad_ref); | ||
| return BISECT_FAILED; | ||
| } | ||
| repo_format_commit_message(the_repository, commit, "%s", &commit_name, | ||
| &pp); | ||
|
|
||
|
|
@@ -806,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc, | |
| */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> When `refs_resolve_ref_unsafe()` is called to resolve HEAD, and returns
> NULL (e.g., HEAD does not exist as a proper ref), the code falls back to
> `repo_get_oid("HEAD")` to try to resolve the OID directly. If that
> succeeds, execution continues with `head` still set to NULL.
>
> Later, that variable is passed to `repo_get_oid()` and `starts_with()`,
> both of which would dereference the NULL pointer.
>
> The scenario "`refs_resolve_ref_unsafe()` returns NULL but
> `repo_get_oid()` succeeds" can happen when HEAD is a detached bare OID
> that the ref backend cannot resolve symbolically (a potential edge case
> with the reftable backend) but the OID itself is valid. In this case,
> the bisect-start file does not yet exist (this is a fresh "git bisect
> start"), so the else branch is taken with the NULL `head`.
I agree that setting head to the string "HEAD" is a good solution to
ensure that !starts_with(), !repo_get_oid(), and skip_prefix() are
not called with NULL.
However, I am not sure I understand your "can happen" scenario.
I naively thought that the only case where HEAD does not resolve to
an object correctly is when HEAD is a symbolic ref pointing to an
unborn branch.
Is the bug in your "can happen" scenario something we can
demonstrate? If so, could you add a test to prevent regressions in
the future?
Thanks.
> Simply assign "HEAD" to `head` as a fallback to address this.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/bisect.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 6ff600c856..a69771c6d3 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -811,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
> */
> head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> "HEAD", 0, &head_oid, &flags);
> - if (!head)
> + if (!head) {
> if (repo_get_oid(the_repository, "HEAD", &head_oid))
> return error(_("bad HEAD - I need a HEAD"));
> + head = "HEAD";
> + }
>
> /*
> * Check if we are bisecting |
||
| head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), | ||
| "HEAD", 0, &head_oid, &flags); | ||
| if (!head) | ||
| if (!head) { | ||
| if (repo_get_oid(the_repository, "HEAD", &head_oid)) | ||
| return error(_("bad HEAD - I need a HEAD")); | ||
| head = "HEAD"; | ||
| } | ||
|
|
||
| /* | ||
| * Check if we are bisecting | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -579,9 +579,13 @@ int cmd_diff(int argc, | |
| obj = deref_tag(the_repository, obj, NULL, 0); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 4b46e394ce..18b1083e98 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -579,9 +579,13 @@ int cmd_diff(int argc,
> obj = deref_tag(the_repository, obj, NULL, 0);
> if (!obj)
> die(_("invalid object '%s' given."), name);
> - if (obj->type == OBJ_COMMIT)
> - obj = &repo_get_commit_tree(the_repository,
> - ((struct commit *)obj))->object;
> + if (obj->type == OBJ_COMMIT) {
> + struct tree *tree = repo_get_commit_tree(
> + the_repository, (struct commit *)obj);
> + if (!tree)
> + die(_("unable to read tree object for commit '%s'"), name);
> + obj = &tree->object;
> + }
Obviously correct.
> if (obj->type == OBJ_TREE) {
> if (sdiff.skip && bitmap_get(sdiff.skip, i)) |
||
| if (!obj) | ||
| die(_("invalid object '%s' given."), name); | ||
| if (obj->type == OBJ_COMMIT) | ||
| obj = &repo_get_commit_tree(the_repository, | ||
| ((struct commit *)obj))->object; | ||
| if (obj->type == OBJ_COMMIT) { | ||
| struct tree *tree = repo_get_commit_tree( | ||
| the_repository, (struct commit *)obj); | ||
| if (!tree) | ||
| die(_("unable to read tree object for commit '%s'"), name); | ||
| obj = &tree->object; | ||
| } | ||
|
|
||
| if (obj->type == OBJ_TREE) { | ||
| if (sdiff.skip && bitmap_get(sdiff.skip, i)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare, | |
| FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 264df6259a..0993418e63 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
> FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
> int file_done = 0;
>
> - if (isatty(fileno(f)))
> - warning(_("reading patches from stdin/tty..."));
> -
> if (!f) {
> error_errno("cannot open mbox %s", file);
> goto out;
> }
>
> + if (isatty(fileno(f)))
> + warning(_("reading patches from stdin/tty..."));
> +
> do {
> peek = fgetc(f);
> if (peek == EOF) {
Ah, obviously correct. Cannot believe nobody noticed this since it
was first written in 7b20af6a06 (am/apply: warn if we end up reading
patches from terminal, 2022-03-03).
Thanks. |
||
| int file_done = 0; | ||
|
|
||
| if (isatty(fileno(f))) | ||
| warning(_("reading patches from stdin/tty...")); | ||
|
|
||
| if (!f) { | ||
| error_errno("cannot open mbox %s", file); | ||
| goto out; | ||
| } | ||
|
|
||
| if (isatty(fileno(f))) | ||
| warning(_("reading patches from stdin/tty...")); | ||
|
|
||
| do { | ||
| peek = fgetc(f); | ||
| if (peek == EOF) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void) | |
| */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The outer loop in `diffcore_merge_broken()` sets `q->queue[j]` to NULL
> when it merges a broken pair back together, and has a NULL check to skip
> such entries on subsequent iterations. The inner loop, however, lacks
> this guard: when it scans forward looking for a matching peer, it can
> encounter a slot that was NULLed by a previous outer-loop iteration and
> dereference it unconditionally.
>
> In practice this requires at least two broken pairs whose peers
> both survive rename/copy detection and appear later in the queue,
> which is rare but not impossible.
Interesting find. This is an ancient part of the codebase that
nobody has touched in the past 21 years since eeaa460314 ([PATCH]
diff: Update -B heuristics., 2005-06-03) introduced it ;-).
Well spotted.
> Add the same `if (!pp) continue` guard to the inner loop.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diffcore-break.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/diffcore-break.c b/diffcore-break.c
> index 17b5ad1fed..b5bcc956cc 100644
> --- a/diffcore-break.c
> +++ b/diffcore-break.c
> @@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
> */
> for (j = i + 1; j < q->nr; j++) {
> struct diff_filepair *pp = q->queue[j];
> + if (!pp)
> + continue;
> if (pp->broken_pair &&
> !strcmp(pp->one->path, pp->two->path) &&
> !strcmp(p->one->path, pp->two->path)) { |
||
| for (j = i + 1; j < q->nr; j++) { | ||
| struct diff_filepair *pp = q->queue[j]; | ||
| if (!pp) | ||
| continue; | ||
| if (pp->broken_pair && | ||
| !strcmp(pp->one->path, pp->two->path) && | ||
| !strcmp(p->one->path, pp->two->path)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, | |
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This can happen in practice with incremental MIDX chains: the base MIDX
> may have been written without `--write-bitmap-index`, or the bitmap may
> have been pruned while the incremental layer's bitmap still references
> it.
>
> Check the return value and go to the cleanup label (which unmaps the
> current bitmap and returns -1) so the caller falls back to non-bitmap
> object enumeration, matching the handling of other bitmap loading
> failures in the same function.
Nicely reasoned. It would have been nicer to CC those who are more
familiar with the area, though.
Cc'ed Taylor for incremental MIDX expertise just in case.
Thanks.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> pack-bitmap.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index e8a82945cc..ca7998c10b 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>
> if (midx->base_midx) {
> bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
> + if (!bitmap_git->base) {
> + warning(_("could not open bitmap for base MIDX"));
> + goto cleanup;
> + }
> bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
> } else {
> bitmap_git->base_nr = 0; |
||
| if (midx->base_midx) { | ||
| bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx); | ||
| if (!bitmap_git->base) { | ||
| warning(_("could not open bitmap for base MIDX")); | ||
| goto cleanup; | ||
| } | ||
| bitmap_git->base_nr = bitmap_git->base->base_nr + 1; | ||
| } else { | ||
| bitmap_git->base_nr = 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st) | |
| st->merged = NULL; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When reftable_new_stack() fails partway through initialization
> (e.g., reftable_buf_addstr returns an OOM error before
> reftable_buf_detach assigns p->list_file), it jumps to the error
> path which calls reftable_stack_destroy(p). At that point,
> p->list_file is still NULL because the detach never happened.
>
> reftable_stack_destroy() passes st->list_file unconditionally to
> read_lines(), which calls open(filename, O_RDONLY). Passing NULL
> to open() is undefined behavior and will typically crash.
>
> Guard the read_lines() call with a NULL check on st->list_file.
> When list_file is NULL, there are no table files to clean up
> anyway, so skipping read_lines is the correct behavior.
Nice spotting and recovery. Well done.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> reftable/stack.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 1fba96ddb3..3fc3c0b2d1 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st)
> st->merged = NULL;
> }
>
> - err = read_lines(st->list_file, &names);
> + if (st->list_file)
> + err = read_lines(st->list_file, &names);
> if (err < 0) {
> REFTABLE_FREE_AND_NULL(names);
> } |
||
| } | ||
|
|
||
| err = read_lines(st->list_file, &names); | ||
| if (st->list_file) | ||
| err = read_lines(st->list_file, &names); | ||
| if (err < 0) { | ||
| REFTABLE_FREE_AND_NULL(names); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname, | |
| { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> However, it requires quite involved reasoning to reach that conclusion,
> and is therefore fragile. Just return -1 ("no tracking ref") when there
> is no remote to work with.
In a case like this, where the function is designed not to be called
with NULL remote, I would prefer to have an explicit BUG() rather
than sweeping the problem under the rug. That would make sure your
investigation and involved reasoning done here remain relevant if
the BUG() triggers due to careless changes to the caller in the
future.
Thanks.
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> remote.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index 00723b385e..34d0367f11 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname,
> {
> char *dst;
>
> + if (!remote)
> + return -1; /* no remote to look up tracking ref */
> dst = apply_refspecs(&remote->fetch, refname);
> if (!dst)
> return -1; /* no tracking ref for refname at remote */ |
||
| char *dst; | ||
|
|
||
| if (!remote) | ||
| BUG("remote_tracking() called with NULL remote"); | ||
| dst = apply_refspecs(&remote->fetch, refname); | ||
| if (!dst) | ||
| return -1; /* no tracking ref for refname at remote */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags, | |
| return 0; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This function resolves revision suffixes like commit^@ (all parents),
> commit^! (commit minus parents), and commit^-N (exclude Nth parent). It
> calls `get_reference()` in a loop to peel through tag objects until it
> reaches a commit.
>
> The existing NULL check after `get_reference()` only handles the
> ignore_missing case, but get_reference() can return NULL through three
> distinct paths:
Nicely spotted. It sounds like something a test can ensure does not
to regress in the future, unless I am misreading this explanation.
Could you include such a test?
Thanks.
> diff --git a/revision.c b/revision.c
> index e91d7e1f11..7f3999b551 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
> return 0;
> while (1) {
> it = get_reference(revs, arg, &oid, 0);
> - if (!it && revs->ignore_missing)
> - return 0;
> + if (!it) {
> + if (revs->ignore_missing)
> + return 0;
> + if (revs->do_not_die_on_missing_objects)
> + return 0;
> + return -1;
> + }
> if (it->type != OBJ_TAG)
> break;
> if (!((struct tag*)it)->tagged) |
||
| while (1) { | ||
| it = get_reference(revs, arg, &oid, 0); | ||
| if (!it && revs->ignore_missing) | ||
| return 0; | ||
| if (!it) { | ||
| if (revs->ignore_missing) | ||
| return 0; | ||
| if (revs->do_not_die_on_missing_objects) | ||
| return 0; | ||
| return -1; | ||
| } | ||
| if (it->type != OBJ_TAG) | ||
| break; | ||
| if (!((struct tag*)it)->tagged) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,7 +359,9 @@ struct write_shallow_data { | |
| static int write_one_shallow(const struct commit_graft *graft, void *cb_data) | ||
| { | ||
| struct write_shallow_data *data = cb_data; | ||
| const char *hex = oid_to_hex(&graft->oid); | ||
| char hex[GIT_MAX_HEXSZ + 1]; | ||
|
|
||
| oid_to_hex_r(hex, &graft->oid); | ||
| if (graft->nr_parent != -1) | ||
| return 0; | ||
| if (data->flags & QUICK) { | ||
|
|
@@ -370,8 +372,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data) | |
| struct commit *c = lookup_commit(the_repository, &graft->oid); | ||
| if (!c || !(c->object.flags & SEEN)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/shallow.c b/shallow.c
> index 07cae44ae5..3d2230351e 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -371,7 +371,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> if (!c || !(c->object.flags & SEEN)) {
> if (data->flags & VERBOSE)
> printf("Removing %s from .git/shallow\n",
> - oid_to_hex(&c->object.oid));
> + oid_to_hex(&graft->oid));
> return 0;
Haha. We come into this block and emit this message when we may not
even have a valid 'c', yet we use c->object.oid there. It makes
perfect sense to use graft->oid here instead, as your patch does.
However, its hexadecimal representation has already been computed in
the local variable 'hex', and the "happy path" code after this
section seems to assume that 'hex' is still valid (even though
oid_to_hex() uses rotating 4-element buffer, which makes the
assumption a risky one).
We should use "hex" here instead of oid_to_hex(&graft->oid), which
does not add to the existing risk. In addition, if we add something
like:
struct write_shallow_data *data = cb_data;
- const char *hex = oid_to_hex(&graft->oid);
+ char hex[GIT_MAX_HEXSZ + 1];
+
+ oid_to_hex_r(hex, &graft->oid);
if (graft->nr_parent != -1)
return 0;
to the beginning of the function, we can get rid of existing
riskiness entirely. |
||
| if (data->flags & VERBOSE) | ||
| printf("Removing %s from .git/shallow\n", | ||
| oid_to_hex(&c->object.oid)); | ||
| printf("Removing %s from .git/shallow\n", hex); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):