coverity: avoid dereferencing NULL#2174
Open
dscho wants to merge 11 commits into
Open
Conversation
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. 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>
The `repo_get_commit_tree()` function can return NULL when a commit's tree object is not available (e.g., the commit was parsed but its maybe_tree field is unset and the commit is not in the commit-graph). In cmd_diff(), the return value is immediately dereferenced via ->object without a NULL check, which would crash if the tree cannot be loaded. Add an explicit NULL check and die with a descriptive message. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `remote_tracking()` function unconditionally dereferences
`remote->fetch` without checking whether remote is NULL.
In practice, this never happens because the only caller (`apply_cas()`)
guards the calls to this function by checking the `use_tracking` and
`use_tracking_for_rest` attributes.
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.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: 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. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `split_mbox()` function calls fileno(f) to check whether the input is a terminal, but the NULL check for f (from `fopen()`) does not happen until later. When the file cannot be opened, f is NULL, and `fileno(NULL)` is undefined behavior, typically crashing with a segmentation fault. Move the NULL check above the `isatty()`/`fileno()` call so the error path is taken before any use of the potentially-NULL handle. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When `lookup_commit_reference_by_name()` is called to find the first bad commit, the result is passed to `repo_format_commit_message()` immediately, which dereferences commit without checking for NULL. However, the commit could be NULL, even though in practice this is unlikely because `bisect_successful()` is only called after a successful bisect run has identified the bad commit, but the ref could still become dangling due to a concurrent gc or repository corruption. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `peel_committish()` function calls `repo_peel_to_type()` to convert the given object to a commit, but does not check the return value. When the object exists but cannot be peeled to a commit (e.g., a tree or blob OID is passed as --onto), the return value is NULL. Add an explicit NULL check and die with a descriptive message in that case. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: 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:
1. revs->ignore_missing: the caller asked to silently skip missing
objects.
2. revs->exclude_promisor_objects: the object is a lazy promisor
object that should be excluded from the walk.
3. revs->do_not_die_on_missing_objects: the caller wants to record
missing OIDs for later reporting (used by `git rev-list
--missing=print`) rather than dying.
In the latter two instances, the code falls through to dereference the
NULL pointer.
Handle all three cases explicitly:
- ignore_missing: return 0, matching the existing behavior and
the pattern in `handle_revision_arg()`.
- do_not_die_on_missing_objects: return 0. The missing OID has already
been recorded in `revs->missing_commits` by `get_reference()`.
Returning 0 is consistent with `handle_revision_arg()` and
`process_parents()`, both of which continue without error when this flag
is set. The broader codebase pattern for this flag is "record and
continue": list-objects.c, builtin/rev-list.c, and process_parents
all skip the die/error and keep walking.
- everything else (only the `exclude_promisor_objects` case in
practice): return -1, consistent with `handle_revision_arg()` where
the condition only matches `ignore_missing` or
`do_not_die_on_missing_objects`, falling through to ret = -1 for the
promisor case.
Note: the callers of `add_parents_only()` in
`handle_revision_pseudo_opt()` treat any nonzero return as "handled"
(`if (add_parents_only(...)) { ret = 0; }`), so the -1 for the promisor
case is indistinguishable from success there. This means a
promisor-excluded tag target referenced via commit^@ would be silently
skipped rather than producing an error. This is a pre-existing
limitation of the caller's return value handling and not made worse by
this change; the alternative (a NULL dereference crash) _would be_
strictly worse.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When `prepare_midx_bitmap_git()` is called to load the bitmap for a chained MIDX's base layer, if the base MIDX does not have an associated bitmap file (e.g., it was not generated, or was deleted by gc), the return value is NULL. It is then stored in `bitmap_git->base` and immediately dereferenced on the next line. 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. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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`.
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>
After `write_one_shallow()` calls `lookup_commit()` to find the commit object for a shallow graft entry, it then checks `if (!c || ...)`. Inside that block, when the VERBOSE flag is set, it prints the OID being removed, via `c->object.oid`. But `c` can be NULL (the first condition in the `||` check). This happens when a shallow graft entry references a commit object that is not in the object store (e.g., after a partial fetch or in a corrupted repository). In that case, `lookup_commit()` returns NULL because the object cannot be found, the SEEN_ONLY check correctly decides to remove this entry from .git/shallow, but the verbose message crashes before the removal can complete. Use `graft->oid` instead of `c->object.oid` for the message. The graft entry's OID is the same value (it was used as the lookup key) and is always available regardless of whether the commit object exists. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2174.git.1783590159.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is a continuation of the effort I started in the patch series that became
js/coverity-fixes. This next batch adds guards to avoid dereferencing NULL pointers and accessing NULL file descriptors.