bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* [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).