| Message ID | RFC-patch-04.15-3a287c19d7e-20220603T183608Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode | expand |
Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason: > Fix a control flow issue dating back to d1f2d7e8ca6 (Make > run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19) > where we'd assume "tree" must be non-NULL if idx was NULL. As > -fanalyzer shows we'd end up dereferencing "tree" in that case in > ce_path_match(): > > dir.h:541:41: warning: dereference of NULL ‘ce’ [CWE-476] [-Wanalyzer-null-dereference] > 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)); > | ^ > ‘oneway_diff’: events 1-2 > | > |diff-lib.c:493:12: > | 493 | static int oneway_diff(const struct cache_entry * const *src, > | | ^~~~~~~~~~~ > | | | > | | (1) entry to ‘oneway_diff’ > |...... > | 506 | if (tree == o->df_conflict_entry) > | | ~ > | | | > | | (2) following ‘true’ branch... > | > ‘oneway_diff’: event 3 > | > | 507 | tree = NULL; > | | ^ > | | | > | | (3) ...to here > | > ‘oneway_diff’: events 4-8 > | > | 507 | tree = NULL; > | | ^ > | | | > | | (4) ‘tree’ is NULL > | 508 | > | 509 | if (ce_path_match(revs->diffopt.repo->index, > | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | | | > | | (5) following ‘false’ branch (when ‘idx’ is NULL)... > | | (6) ...to here > | | (7) ‘tree’ is NULL > | | (8) calling ‘ce_path_match’ from ‘oneway_diff’ > | 510 | idx ? idx : tree, > | | ~~~~~~~~~~~~~~~~~ > | 511 | &revs->prune_data, NULL)) { > | | ~~~~~~~~~~~~~~~~~~~~~~~~ > | > +--> ‘ce_path_match’: event 9 > | > |dir.h:535:19: > | 535 | static inline int ce_path_match(struct index_state *istate, > | | ^~~~~~~~~~~~~ > | | | > | | (9) entry to ‘ce_path_match’ > | > ‘ce_path_match’: event 10 > | > | 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)); > | | ^ > | | | > | | (10) dereference of NULL ‘ce > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff-lib.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/diff-lib.c b/diff-lib.c > index ca085a03efc..8373ad7e3ea 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -506,6 +506,9 @@ static int oneway_diff(const struct cache_entry * const *src, > if (tree == o->df_conflict_entry) > tree = NULL; So here we have a D/F conflict in a oneway diff, i.e. a single tree. That means if we discard the thing from the tree, then we still have the conflicting thing from the index. Meaning idx and tree cannot both be NULL in that D/F conflict scenario. Right? > > + if (!idx && !tree) > + return 0; That calms down the confused compiler, but would it perhaps be better to BUG out at this point? Or is there a valid state with both idx and tree being NULL? > + > if (ce_path_match(revs->diffopt.repo->index, > idx ? idx : tree,> &revs->prune_data, NULL)) {
diff --git a/diff-lib.c b/diff-lib.c index ca085a03efc..8373ad7e3ea 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -506,6 +506,9 @@ static int oneway_diff(const struct cache_entry * const *src, if (tree == o->df_conflict_entry) tree = NULL; + if (!idx && !tree) + return 0; + if (ce_path_match(revs->diffopt.repo->index, idx ? idx : tree, &revs->prune_data, NULL)) {
Fix a control flow issue dating back to d1f2d7e8ca6 (Make run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19) where we'd assume "tree" must be non-NULL if idx was NULL. As -fanalyzer shows we'd end up dereferencing "tree" in that case in ce_path_match(): dir.h:541:41: warning: dereference of NULL ‘ce’ [CWE-476] [-Wanalyzer-null-dereference] 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)); | ^ ‘oneway_diff’: events 1-2 | |diff-lib.c:493:12: | 493 | static int oneway_diff(const struct cache_entry * const *src, | | ^~~~~~~~~~~ | | | | | (1) entry to ‘oneway_diff’ |...... | 506 | if (tree == o->df_conflict_entry) | | ~ | | | | | (2) following ‘true’ branch... | ‘oneway_diff’: event 3 | | 507 | tree = NULL; | | ^ | | | | | (3) ...to here | ‘oneway_diff’: events 4-8 | | 507 | tree = NULL; | | ^ | | | | | (4) ‘tree’ is NULL | 508 | | 509 | if (ce_path_match(revs->diffopt.repo->index, | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (5) following ‘false’ branch (when ‘idx’ is NULL)... | | (6) ...to here | | (7) ‘tree’ is NULL | | (8) calling ‘ce_path_match’ from ‘oneway_diff’ | 510 | idx ? idx : tree, | | ~~~~~~~~~~~~~~~~~ | 511 | &revs->prune_data, NULL)) { | | ~~~~~~~~~~~~~~~~~~~~~~~~ | +--> ‘ce_path_match’: event 9 | |dir.h:535:19: | 535 | static inline int ce_path_match(struct index_state *istate, | | ^~~~~~~~~~~~~ | | | | | (9) entry to ‘ce_path_match’ | ‘ce_path_match’: event 10 | | 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)); | | ^ | | | | | (10) dereference of NULL ‘ce Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- diff-lib.c | 3 +++ 1 file changed, 3 insertions(+)