* [PATCH 0/3] Some Coverity fixes from GRUB
@ 2021-06-18 15:44 Darren Kenny
2021-06-18 15:44 ` [PATCH 1/3] lib/regexec: Fix possible null-dereference Darren Kenny
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Darren Kenny @ 2021-06-18 15:44 UTC (permalink / raw)
To: bug-gnulib; +Cc: darren.kenny
During the recent GRUB releases, I took a look at some Coverity issues that
were found in the gnulib sources that GRUB is using as part of its build.
A couple of these issues still exist in the current gnulib master branch
so I've put the fixes for these together to in this patchset.
I did a build using the command:
./gnulib-tool \
--with-tests \
--dir=/tmp/gnulib-test \
--test argp base64 error fnmatch getdelim \
getline gettext-h gitlog-to-changelog \
mbswidth progname realloc-gnu \
regex save-cwd
which is fairly close to the list of modules that GRUB is using and there
were not test failures.
Thanks,
Darren.
Darren Kenny (3):
lib/regexec: Fix possible null-dereference
lib/argp-help: Fix possible dereference of a NULL state
lib/regexec: Resolve unused variable
lib/argp-help.c | 3 ++-
lib/regexec.c | 7 +++++++
2 files changed, 9 insertions(+), 1 deletion(-)
--
2.18.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-06-18 15:44 [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
@ 2021-06-18 15:44 ` Darren Kenny
2021-08-11 7:28 ` Paul Eggert
2021-06-18 15:44 ` [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state Darren Kenny
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Darren Kenny @ 2021-06-18 15:44 UTC (permalink / raw)
To: bug-gnulib; +Cc: darren.kenny
It appears to be possible that the mctx->state_log field may be NULL,
and the name of this function, clean_state_log_if_needed(), suggests
that it should be checking that it is valid to be cleaned before
assuming that it does.
This was originally found during a Coverity scan of GRUB2.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
lib/regexec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/regexec.c b/lib/regexec.c
index 5d4113c9d3ee..2b2ab8e4afd0 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -1672,6 +1672,9 @@ clean_state_log_if_needed (re_match_context_t *mctx, Idx next_state_log_idx)
{
Idx top = mctx->state_log_top;
+ if (mctx->state_log == NULL)
+ return REG_NOERROR;
+
if ((next_state_log_idx >= mctx->input.bufs_len
&& mctx->input.bufs_len < mctx->input.len)
|| (next_state_log_idx >= mctx->input.valid_len
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state
2021-06-18 15:44 [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
2021-06-18 15:44 ` [PATCH 1/3] lib/regexec: Fix possible null-dereference Darren Kenny
@ 2021-06-18 15:44 ` Darren Kenny
2021-06-18 17:36 ` Bruno Haible
2021-06-18 15:44 ` [PATCH 3/3] lib/regexec: Resolve unused variable Darren Kenny
2021-08-06 14:29 ` [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
3 siblings, 1 reply; 16+ messages in thread
From: Darren Kenny @ 2021-06-18 15:44 UTC (permalink / raw)
To: bug-gnulib; +Cc: darren.kenny
All other instances of call to __argp_failure() where there is
a dgettext() call first check whether the valie of state is NULL
before attempting to dereference it to get the root_argp->argp_domain.
This was originally found during a Coverity scan of GRUB2.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
lib/argp-help.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/argp-help.c b/lib/argp-help.c
index 4c89697bdd05..80cdb44937d3 100644
--- a/lib/argp-help.c
+++ b/lib/argp-help.c
@@ -147,7 +147,8 @@ validate_uparams (const struct argp_state *state, struct uparams *upptr)
if (*(int *)((char *)upptr + up->uparams_offs) >= upptr->rmargin)
{
__argp_failure (state, 0, 0,
- dgettext (state->root_argp->argp_domain,
+ dgettext (state == NULL ? NULL
+ : state->root_argp->argp_domain,
"\
ARGP_HELP_FMT: %s value is less than or equal to %s"),
"rmargin", up->name);
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] lib/regexec: Resolve unused variable
2021-06-18 15:44 [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
2021-06-18 15:44 ` [PATCH 1/3] lib/regexec: Fix possible null-dereference Darren Kenny
2021-06-18 15:44 ` [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state Darren Kenny
@ 2021-06-18 15:44 ` Darren Kenny
2021-08-11 7:24 ` Paul Eggert
2021-08-06 14:29 ` [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
3 siblings, 1 reply; 16+ messages in thread
From: Darren Kenny @ 2021-06-18 15:44 UTC (permalink / raw)
To: bug-gnulib; +Cc: darren.kenny
This is a fairly minor issue where a variable is being assigned to but
not checked before it is overwritten again.
The reason for this issue is that we are not building with DEBUG set and
this in turn means that the assert() that reads the value of the
variable match_last is being processed out.
The solution, move the assignment to match_last in to an ifdef DEBUG too.
This was originally found during a Coverity scan of GRUB2.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
lib/regexec.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/regexec.c b/lib/regexec.c
index 2b2ab8e4afd0..ed3fa43bd0d1 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -816,7 +816,11 @@ re_search_internal (const regex_t *preg, const char *string, Idx length,
break;
if (__glibc_unlikely (err != REG_NOMATCH))
goto free_return;
+#ifdef DEBUG
+ /* Only used for assertion below when DEBUG is set, otherwise
+ it will be over-written when we loop around. */
match_last = -1;
+#endif
}
else
break; /* We found a match. */
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state
2021-06-18 15:44 ` [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state Darren Kenny
@ 2021-06-18 17:36 ` Bruno Haible
2021-06-21 9:32 ` Darren Kenny
0 siblings, 1 reply; 16+ messages in thread
From: Bruno Haible @ 2021-06-18 17:36 UTC (permalink / raw)
To: bug-gnulib; +Cc: Darren Kenny
Darren Kenny wrote:
> All other instances of call to __argp_failure() where there is
> a dgettext() call first check whether the valie of state is NULL
> before attempting to dereference it to get the root_argp->argp_domain.
>
> This was originally found during a Coverity scan of GRUB2.
Thanks. I confirm that that is a possible NULL dereference here. I've
applied your patch.
The notation '(tiny change) is explained in
<https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html>.
2021-06-18 Darren Kenny <darren.kenny@oracle.com> (tiny change)
argp: Avoid possible NULL access in argp_help.
Reported by Coverity. The invocation chain is:
argp_help -> _help -> fill_in_uparams -> validate_uparams.
* lib/argp-help.c (validate_uparams): Don't crash if state == NULL.
diff --git a/lib/argp-help.c b/lib/argp-help.c
index 4c89697..80cdb44 100644
--- a/lib/argp-help.c
+++ b/lib/argp-help.c
@@ -147,7 +147,8 @@ validate_uparams (const struct argp_state *state, struct uparams *upptr)
if (*(int *)((char *)upptr + up->uparams_offs) >= upptr->rmargin)
{
__argp_failure (state, 0, 0,
- dgettext (state->root_argp->argp_domain,
+ dgettext (state == NULL ? NULL
+ : state->root_argp->argp_domain,
"\
ARGP_HELP_FMT: %s value is less than or equal to %s"),
"rmargin", up->name);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state
2021-06-18 17:36 ` Bruno Haible
@ 2021-06-21 9:32 ` Darren Kenny
0 siblings, 0 replies; 16+ messages in thread
From: Darren Kenny @ 2021-06-21 9:32 UTC (permalink / raw)
To: Bruno Haible, bug-gnulib
On Friday, 2021-06-18 at 19:36:55 +02, Bruno Haible wrote:
> Darren Kenny wrote:
>> All other instances of call to __argp_failure() where there is
>> a dgettext() call first check whether the valie of state is NULL
>> before attempting to dereference it to get the root_argp->argp_domain.
>>
>> This was originally found during a Coverity scan of GRUB2.
>
> Thanks. I confirm that that is a possible NULL dereference here. I've
> applied your patch.
>
> The notation '(tiny change) is explained in
> <https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html>.
>
>
> 2021-06-18 Darren Kenny <darren.kenny@oracle.com> (tiny change)
>
> argp: Avoid possible NULL access in argp_help.
> Reported by Coverity. The invocation chain is:
> argp_help -> _help -> fill_in_uparams -> validate_uparams.
> * lib/argp-help.c (validate_uparams): Don't crash if state == NULL.
>
> diff --git a/lib/argp-help.c b/lib/argp-help.c
> index 4c89697..80cdb44 100644
> --- a/lib/argp-help.c
> +++ b/lib/argp-help.c
> @@ -147,7 +147,8 @@ validate_uparams (const struct argp_state *state, struct uparams *upptr)
> if (*(int *)((char *)upptr + up->uparams_offs) >= upptr->rmargin)
> {
> __argp_failure (state, 0, 0,
> - dgettext (state->root_argp->argp_domain,
> + dgettext (state == NULL ? NULL
> + : state->root_argp->argp_domain,
> "\
> ARGP_HELP_FMT: %s value is less than or equal to %s"),
> "rmargin", up->name);
Thanks Bruno.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] Some Coverity fixes from GRUB
2021-06-18 15:44 [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
` (2 preceding siblings ...)
2021-06-18 15:44 ` [PATCH 3/3] lib/regexec: Resolve unused variable Darren Kenny
@ 2021-08-06 14:29 ` Darren Kenny
3 siblings, 0 replies; 16+ messages in thread
From: Darren Kenny @ 2021-08-06 14:29 UTC (permalink / raw)
To: bug-gnulib; +Cc: Daniel Kiper
Hi,
I've not heard anything on the first or third patches in this series, is
there any possiblity that these can be take in to gnulib?
Bruno has already taken patch 2 into the gnulib tree, thanks.
If there is anything more that is needed to get them in, please let me
know since we would like to be able to bring in the latest gnulib
version to GRUB, and remove these patches from the GRUB tree.
Thanks,
Darren.
On Friday, 2021-06-18 at 15:44:20 UTC, Darren Kenny wrote:
> During the recent GRUB releases, I took a look at some Coverity issues that
> were found in the gnulib sources that GRUB is using as part of its build.
>
> A couple of these issues still exist in the current gnulib master branch
> so I've put the fixes for these together to in this patchset.
>
> I did a build using the command:
>
> ./gnulib-tool \
> --with-tests \
> --dir=/tmp/gnulib-test \
> --test argp base64 error fnmatch getdelim \
> getline gettext-h gitlog-to-changelog \
> mbswidth progname realloc-gnu \
> regex save-cwd
>
> which is fairly close to the list of modules that GRUB is using and there
> were not test failures.
>
> Thanks,
>
> Darren.
>
> Darren Kenny (3):
> lib/regexec: Fix possible null-dereference
> lib/argp-help: Fix possible dereference of a NULL state
> lib/regexec: Resolve unused variable
>
> lib/argp-help.c | 3 ++-
> lib/regexec.c | 7 +++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> --
> 2.18.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] lib/regexec: Resolve unused variable
2021-06-18 15:44 ` [PATCH 3/3] lib/regexec: Resolve unused variable Darren Kenny
@ 2021-08-11 7:24 ` Paul Eggert
2021-08-23 10:38 ` Darren Kenny
0 siblings, 1 reply; 16+ messages in thread
From: Paul Eggert @ 2021-08-11 7:24 UTC (permalink / raw)
To: Darren Kenny, bug-gnulib
On 6/18/21 8:44 AM, Darren Kenny wrote:
> The reason for this issue is that we are not building with DEBUG set and
> this in turn means that the assert() that reads the value of the
> variable match_last is being processed out.
Unfortunately I don't understand the scenario here. If not building with
DEBUG, 'DEBUG_ASSERT (match_last != 1)' should expand to 'assume
(match_last != 1)', which in turn should expand to something that
evaluates the expression 'match_last != 1'. Please see this commit,
which removed the "#ifdef" that you're proposing to re-add:
https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=79f8ee4e389f8cb1339f8abed9a7d29816e2a2d4
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-06-18 15:44 ` [PATCH 1/3] lib/regexec: Fix possible null-dereference Darren Kenny
@ 2021-08-11 7:28 ` Paul Eggert
2021-08-23 10:05 ` Darren Kenny
0 siblings, 1 reply; 16+ messages in thread
From: Paul Eggert @ 2021-08-11 7:28 UTC (permalink / raw)
To: Darren Kenny; +Cc: bug-gnulib
[-- Attachment #1: Type: text/plain, Size: 461 bytes --]
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.
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 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.
[-- Attachment #2: coverity.diff --]
[-- Type: text/x-patch, Size: 452 bytes --]
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-08-11 7:28 ` Paul Eggert
@ 2021-08-23 10:05 ` Darren Kenny
2021-08-23 20:09 ` Paul Eggert
0 siblings, 1 reply; 16+ messages in thread
From: Darren Kenny @ 2021-08-23 10:05 UTC (permalink / raw)
To: Paul Eggert; +Cc: Daniel Kiper, bug-gnulib
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] lib/regexec: Resolve unused variable
2021-08-11 7:24 ` Paul Eggert
@ 2021-08-23 10:38 ` Darren Kenny
2021-08-23 13:00 ` Bruno Haible
2021-08-23 21:04 ` Paul Eggert
0 siblings, 2 replies; 16+ messages in thread
From: Darren Kenny @ 2021-08-23 10:38 UTC (permalink / raw)
To: Paul Eggert, bug-gnulib
Hi Paul,
On Wednesday, 2021-08-11 at 00:24:25 -07, Paul Eggert wrote:
> On 6/18/21 8:44 AM, Darren Kenny wrote:
>> The reason for this issue is that we are not building with DEBUG set and
>> this in turn means that the assert() that reads the value of the
>> variable match_last is being processed out.
>
> Unfortunately I don't understand the scenario here. If not building with
> DEBUG, 'DEBUG_ASSERT (match_last != 1)' should expand to 'assume
> (match_last != 1)', which in turn should expand to something that
> evaluates the expression 'match_last != 1'. Please see this commit,
> which removed the "#ifdef" that you're proposing to re-add:
>
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=79f8ee4e389f8cb1339f8abed9a7d29816e2a2d4
The description of the problem by Coverity is:
- An assigned value that is never used may represent unnecessary
computation, an incorrect algorithm, or possibly the need for cleanup or
refactoring. In re_search_internal: A value assigned to a variable is
never used. (CWE-563)
That it is referring to in this case is that this assignment is being
made to set match_last = -1, but before ever testing it with-in the
for-loop, it is overwiting that value.
So, the only testing was occurring outside of the for-loop, and in what
appeared to be an assertion only fired during DEBUG.
What I did here was to not overwrite the value if DEBUG is set,
since it appeared to never be checked anyway within the for-loop.
I don't know about DEBUG_ASSERT() becoming an assume() - I can't even
find a reference to assume() in the GCC manuals, only assert(),
similarly for the gnulib manual's index at:
- https://www.gnu.org/software/gnulib/manual/html_node/Index.html#Index_cp_letter-A
I can see that LLVM has a __builtin_assume() though, but that's about
the closest I could find.
Searching through the gnulib sources did find verify.h and the assume
macro defined in that - I can only assume (pun not intended) that this
is ending up as a no-op for our build, which means that Coverity sees it
as never being read after the for-loop.
Thanks,
Darren
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] lib/regexec: Resolve unused variable
2021-08-23 10:38 ` Darren Kenny
@ 2021-08-23 13:00 ` Bruno Haible
2021-08-23 21:04 ` Paul Eggert
1 sibling, 0 replies; 16+ messages in thread
From: Bruno Haible @ 2021-08-23 13:00 UTC (permalink / raw)
To: Paul Eggert, bug-gnulib; +Cc: Darren Kenny
Darren Kenny wrote:
> I can see that LLVM has a __builtin_assume() though, but that's about
> the closest I could find.
We avoid clang's __builtin_assume, because it triggers the generation of
wrong code. See
<https://lists.gnu.org/archive/html/bug-gnulib/2020-09/msg00014.html>.
Bruno
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-08-23 10:05 ` Darren Kenny
@ 2021-08-23 20:09 ` Paul Eggert
2021-08-24 15:11 ` Darren Kenny
0 siblings, 1 reply; 16+ messages in thread
From: Paul Eggert @ 2021-08-23 20:09 UTC (permalink / raw)
To: Darren Kenny; +Cc: Daniel Kiper, bug-gnulib
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] lib/regexec: Resolve unused variable
2021-08-23 10:38 ` Darren Kenny
2021-08-23 13:00 ` Bruno Haible
@ 2021-08-23 21:04 ` Paul Eggert
1 sibling, 0 replies; 16+ messages in thread
From: Paul Eggert @ 2021-08-23 21:04 UTC (permalink / raw)
To: Darren Kenny; +Cc: bug-gnulib
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
On 8/23/21 3:38 AM, Darren Kenny wrote:
> What I did here was to not overwrite the value if DEBUG is set,
> since it appeared to never be checked anyway within the for-loop.
Yes, but although your patch pacified Coverity I still don't understand
exactly why. I'd like to get to the bottom of it before continuing.
> I can only assume (pun not intended) that this
> is ending up as a no-op for our build, which means that Coverity sees it
> as never being read after the for-loop.
I'm working blind here, since Coverity's documentation is secret, which
means you'll need to help out by reading its documentation and/or
running Coverity yourself. Can you do that and let us know the following?
* What values does Coverity define the macros __GNUC__ and
__GNUC_MINOR__ to? Or does it leave these symbols undefined?
* Similarly for _MSC_VER (defined by Microsoft C compilers).
* Does Coverity support the __builtin_unreachable and/or __builtin_trap
functions, like GCC does? (See their use in verify.h.)
* Does Coverity support __assume, like Microsoft C does? (Also see
verify.h.)
* Does Coverity support __has_builtin, like GCC and Clang do? (Again,
see verify.h.)
* Are you compiling with -DGCC_LINT and/or -Dlint?
* Does the attached patch work for you? If not, why not, and what sort
of improvements could be made?
Thanks.
[-- Attachment #2: coverity2.diff --]
[-- Type: text/x-patch, Size: 576 bytes --]
diff --git a/lib/verify.h b/lib/verify.h
index a8ca59b09..3cd71b280 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -305,6 +305,8 @@ template <int w>
--enable-gcc-warnings, which compiles with -Dlint. It's nicer
when 'assume' silences warnings even with older GCCs. */
# define assume(R) ((R) ? (void) 0 : __builtin_trap ())
+#elif defined __COVERITY__
+# define assume(R) ((R) ? (void) 0 : __coverity_panic__ ())
#else
/* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6. */
# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-08-23 20:09 ` Paul Eggert
@ 2021-08-24 15:11 ` Darren Kenny
2021-08-24 18:52 ` Paul Eggert
0 siblings, 1 reply; 16+ messages in thread
From: Darren Kenny @ 2021-08-24 15:11 UTC (permalink / raw)
To: Paul Eggert; +Cc: Daniel Kiper, bug-gnulib
On Monday, 2021-08-23 at 13:09:18 -07, Paul Eggert wrote:
> 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?
>
You're right, I should have fetched from HEAD, the version I was looking
at was several weeks old at changeset b50a7e59debf8.
>> - 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.
>
It certainly may be that Coverity just doesn't know enough for all
use-cases.
Is there a specific set of assertions already present that ensure that
the circumstances you outline above are always in place?
>> 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.
>
At present at least, we're not building GRUB with DEBUG, but maybe it is
something to consider for Coverity builds.
Thanks,
Darren.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] lib/regexec: Fix possible null-dereference
2021-08-24 15:11 ` Darren Kenny
@ 2021-08-24 18:52 ` Paul Eggert
0 siblings, 0 replies; 16+ messages in thread
From: Paul Eggert @ 2021-08-24 18:52 UTC (permalink / raw)
To: Darren Kenny; +Cc: Daniel Kiper, bug-gnulib
[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]
On 8/24/21 8:11 AM, Darren Kenny wrote:
> Is there a specific set of assertions already present that ensure that
> the circumstances you outline above are always in place?
If by "assertion" you mean using DEBUG_ASSERT to pacify Coverity, I
think that the answer is currently "no" but if you apply something like
the attached patch then the answer becomes "yes". You'll have to check
this though, as Coverity won't give me access to its tools or source code.
If by "assertion" you mean using <assert.h> to verify each of the
statements I made, then this is not the sort of thing that one can
easily ensure via assertions. It'd be like asking "Is there a specific
set of <assert.h> assertions that I can add to 'grep' to ensure that the
output of 'grep' is correct?" The answer to that question is "No,
there's no practical way to do it."
> At present at least, we're not building GRUB with DEBUG, but maybe it is
> something to consider for Coverity builds.
With the attached patch, I hope your Coverity builds can be either with
or without DEBUG.
[-- Attachment #2: coverity3.diff --]
[-- Type: text/x-patch, Size: 1010 bytes --]
diff --git a/lib/regexec.c b/lib/regexec.c
index 5e4eb497a..bd5b4ea41 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -1675,6 +1675,7 @@ static reg_errcode_t
clean_state_log_if_needed (re_match_context_t *mctx, Idx next_state_log_idx)
{
Idx top = mctx->state_log_top;
+ DEBUG_ASSERT (mctx->state_log != NULL);
if ((next_state_log_idx >= mctx->input.bufs_len
&& mctx->input.bufs_len < mctx->input.len)
diff --git a/lib/verify.h b/lib/verify.h
index a8ca59b09..3cd71b280 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -305,6 +305,8 @@ template <int w>
--enable-gcc-warnings, which compiles with -Dlint. It's nicer
when 'assume' silences warnings even with older GCCs. */
# define assume(R) ((R) ? (void) 0 : __builtin_trap ())
+#elif defined __COVERITY__
+# define assume(R) ((R) ? (void) 0 : __coverity_panic__ ())
#else
/* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6. */
# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-08-24 18:52 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 15:44 [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
2021-06-18 15:44 ` [PATCH 1/3] lib/regexec: Fix possible null-dereference Darren Kenny
2021-08-11 7:28 ` Paul Eggert
2021-08-23 10:05 ` Darren Kenny
2021-08-23 20:09 ` Paul Eggert
2021-08-24 15:11 ` Darren Kenny
2021-08-24 18:52 ` Paul Eggert
2021-06-18 15:44 ` [PATCH 2/3] lib/argp-help: Fix possible dereference of a NULL state Darren Kenny
2021-06-18 17:36 ` Bruno Haible
2021-06-21 9:32 ` Darren Kenny
2021-06-18 15:44 ` [PATCH 3/3] lib/regexec: Resolve unused variable Darren Kenny
2021-08-11 7:24 ` Paul Eggert
2021-08-23 10:38 ` Darren Kenny
2021-08-23 13:00 ` Bruno Haible
2021-08-23 21:04 ` Paul Eggert
2021-08-06 14:29 ` [PATCH 0/3] Some Coverity fixes from GRUB Darren Kenny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).