Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference

[Paul Eggert <eggert@cs.ucla.edu>]

On 8/23/21 3:05 AM, Darren Kenny wrote:

> The explanation given by Coverity boils down to:
> 
> - In check_matching(), line 1069, there is a test if mctx->state_log !=
>    NULL

This line number doesn't match either the current Gnulib version (commit 
d3837928885e91c9ddd465240b90a97aa342fda6) nor the version in the current 
Grub release (2.06). So I guess you are using some other version of 
regexec.c. Could you tell us which one?

> - In transit_state(), there is a call to transit_state_mb() at line
>    2226, which in turn, at line 2497, directly references mctx->state_log
>    without first checking if it is NULL.

In the Gnulib version, transit_state calls transit_state_mb only if 
state->accept_mb is true, and if the state can accept multibyte 
characters then in re_search_internal dfa->has_mb_node must be true, 
which means that re_search_internal initializes mctx.state_log to a 
nonnull pointer before we get to transit_state.

So I'm not seeing a bug here; it still appears to be a false alarm. If 
I'm missing something please let us know.

> The patch we have already satisfies Coverity, once applied

Yes, I can see why the patch would pacify Coverity. However, we 
shouldn't add unnecessary code merely to pacify a Coverity false alarm.

> I have not checked if a DEBUG_ASSERT() call, in a path that Coverity isn't
> including anywhere in its analysis would work.
> 
> Why do you think an assert in clean_state_log_if_needed() would help?

If you tell Coverity to analyze with -DDEBUG, then adding DEBUG_ASSERT 
(X != NULL) should tell Coverity that X must be nonnull at that point. 
We can use this method to tell Coverity things that it can't deduce on 
its own.