Re: [RFC PATCH 04/15] diff-lib.c: don't dereference NULL in oneway_diff()

["René Scharfe" <l.s.r@web.de>]

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)) {