[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
From: |
Paul Eggert |
Subject: |
Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference |
Date: |
Mon, 23 Aug 2021 13:09:18 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
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.