[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
From: |
Darren Kenny |
Subject: |
Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference |
Date: |
Mon, 23 Aug 2021 11:05:13 +0100 |
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