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

[Darren Kenny <darren.kenny@oracle.com>]

Hi Paul,

Sorry, somehow I missed your responses.

On Wednesday, 2021-08-11 at 00:28:26 -07, Paul Eggert wrote:
> On 6/18/21 8:44 AM, Darren Kenny wrote:
>> It appears to be possible that the mctx->state_log field may be NULL
>
> How so? I don't see the execution path that would do that.
>

The explanation given by Coverity boils down to:

- In check_matching(), line 1069, there is a test if mctx->state_log !=
  NULL

- Later there is a call to transit_state() at line 1125, which is
  followed by a check for mctx->state_log != NULL again.

- 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.

Because of the existing checks if mctx->state_log != NULL, Coverity
seems to be assuming that it is possible that it may be NULL, so
transit_state_mb()'s use of it should also be checking it.

Coverity also acknowledges that extend_buffers() may also modify the
field, but not in all cases.

> If you can see how it could happen, please let us know. Otherwise, does 
> the attached patch pacify Coverity, and if not why not?
>

The patch we have already satisfies Coverity, once applied, 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?

Thanks,

Darren.

> The DEBUG_ASSERT stuff does pacify GCC, as it tells GCC things that GCC 
> isn't smart enough to figure out on its own. I hope Coverity can use 
> similar advice.
> diff --git a/lib/regexec.c b/lib/regexec.c
> index 5e4eb497a..f25e00d83 100644
> --- a/lib/regexec.c
> +++ b/lib/regexec.c
> @@ -1674,6 +1674,8 @@ build_sifted_states (const re_match_context_t *mctx, re_sift_context_t *sctx,
>  static reg_errcode_t
>  clean_state_log_if_needed (re_match_context_t *mctx, Idx next_state_log_idx)
>  {
> +  DEBUG_ASSERT (mctx->state_log != NULL);
> +
>    Idx top = mctx->state_log_top;
>  
>    if ((next_state_log_idx >= mctx->input.bufs_len