Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On 12/7/21 11:44, Robbie Harwood wrote: it's odd to me to hear disabling it being the suggested course of action (especially given it's in the gcc -Wextra set, not some random flag). Not odd at all. High-quality static-checking tools have tons of options for a reason, and one shouldn't assume that the tools' defaults are right for your project because (assuming the project is sufficiently complex, which GDB certainly is) they probably aren't. Part of the problem here is that there are two conflicting attitudes about types like size_t. The traditional attitude is that one should compute sizes using size_t arithmetic, and be very careful about integer overflow because it's well-defined behavior in C so your runtime won't report it and your program will likely misbehave in mysterious ways later. Gnulib was originally written that way; however, some of its modules have been redone to follow a newer approach, in which one should compute size using ptrdiff_t arithmetic, and (while still being very careful about overflow) hope that your runtime will report it right away so that bugs are more likely to be caught and can be fixed more easily. This latter hope is realized with gcc -fsanitize=undefined, so this is a win over the traditional approach. gcc's -Wsign-compare flag was designed more with the traditional style in mind, and it hasn't really caught up with the newer style. I hope GCC catches up someday; in the meantime, I've found it better to disable -Wsign-compare than to pacify it, as pacification makes code harder to maintain (and so, likely buggier) and can mask some other errors. As you point out with coreutils, gnulib taking a position like this on flags has the knock-on effect that, for our grub, we'll need to do one of the following: - Carry a gnulib patch to make the flag work (what we've been doing). - Change flags in the outer work (i.e., change build options for grub) - Maintain logic to keep flags gnulib-disliked flags out when building the gnulib modules Every other Gnulib-using project I know takes the third approach.
Re: [PATCH v2 05/10] Make gnulib's regcomp not abort()
On 12/7/21 10:51, Robbie Harwood wrote: I don't believe we have an implementation of abort() that can be called. (We have grub_abort() instead.) If that's the correct reason, then DEBUG_ASSERT would work and I can make that change. Looking into the code a bit more, it looks like a DEBUG_ASSERT would not be appropriate at least for the first 'abort ()' since the code is trying to make glibc regerror more user- and debugger-friendly, and using DEBUG_ASSERT would make it less friendly. Can you compile regexec.c with -Dabort=gnu_abort instead?
Re: [PATCH v2 02/10] gnulib/regexec: Fix possible null-dereference
On 12/7/21 09:38, Robbie Harwood wrote:
My*guess* is that Coverity has noticed that `mctx->state_log` is
checked against NULL in many other places in that file, and was unable
to prove to itself that it couldn't be NULL there too. If that's the
case, a DEBUG_ASSERT would presumably do the trick better.
Yes, I can see why Coverity can't deduce the code is safe.
I installed the attached patch into Gnulib; it adds a DEBUG_ASSERT which
should be a reasonable prophylactic even if we don't use Coverity. I
hope this also suffices to pacify Coverity.
I put the DEBUG_ASSERT in a different place, since we shouldn't need to
worry about the assertion unless top < next_state_log_idx.From 12df33fed758c4cb71b8ae38b973657ca941faec Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Tue, 7 Dec 2021 14:34:21 -0800
Subject: [PATCH] regex: pacify Coverity clean_state_log_if_needed
Problem reported by Robbie Harwood in:
https://lists.gnu.org/r/bug-gnulib/2021-12/msg5.html
* lib/regexec.c (clean_state_log_if_needed):
Add a DEBUG_ASSERT; this both pacifies Coverity and
will help to debug in case some other change mistakenly
causes the assertion to become false.
---
ChangeLog | 10 ++
lib/regexec.c | 1 +
2 files changed, 11 insertions(+)
diff --git a/ChangeLog b/ChangeLog
index d2e96eb7f7..bb3edbe44a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2021-12-07 Paul Eggert
+
+ regex: pacify Coverity clean_state_log_if_needed
+ Problem reported by Robbie Harwood in:
+ https://lists.gnu.org/r/bug-gnulib/2021-12/msg5.html
+ * lib/regexec.c (clean_state_log_if_needed):
+ Add a DEBUG_ASSERT; this both pacifies Coverity and
+ will help to debug in case some other change mistakenly
+ causes the assertion to become false.
+
2021-12-07 Bruno Haible
gettext-h: Optimize also for clang.
diff --git a/lib/regexec.c b/lib/regexec.c
index 085bf27b09..3196708373 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -1670,6 +1670,7 @@ clean_state_log_if_needed (re_match_context_t *mctx, Idx next_state_log_idx)
if (top < next_state_log_idx)
{
+ DEBUG_ASSERT (mctx->state_log != NULL);
memset (mctx->state_log + top + 1, '\0',
sizeof (re_dfastate_t *) * (next_state_log_idx - top));
mctx->state_log_top = next_state_log_idx;
--
2.33.1
Re: [PATCH v2 01/10] argp-parse.c (__argp_input): Don't crash if pstate is NULL
Colin Watson writes:
> On Tue, Dec 07, 2021 at 11:59:01AM -0500, Robbie Harwood wrote:
>> Bruno Haible writes:
>> > I don't think this patch is needed, because:
>> >
>> > 1) The application cannot construct a 'struct argp_state' by itself, since
>> > [1]
>> >says that the 'struct argp_state' contains a member 'pstate' that is
>> >"Private, for use by the argp implementation.".
>> >
>> > 2) The only place in the gnulib / glibc code where a 'struct argp_state' is
>> >being constructed, is in function parser_init, invoked from
>> > 'argp_parse',
>> >and there a non-NULL value is assigned.
>> >
>> > In other words, there is no way, compliant with the documented API, that a
>> > NULL pointer can arise as state->pstate.
>> >
>> > Bruno
>> >
>> > [1]
>> > https://www.gnu.org/software/libc/manual/html_node/Argp-Parsing-State.html
>>
>> Thanks for taking a look. I don't have more information on this one
>> beyond the little that's in the commit, so unless Colin remembers why
>> this was needed in 2011, I'll propose grub drop it at some point.
>
> The original problem was:
>
> https://bugs.debian.org/612692
>
> At the time, __argp_help looked like this:
>
> void __argp_help (const struct argp *argp, FILE *stream,
> unsigned flags, char *name)
> {
> struct argp_state state;
> memset (, 0, sizeof state);
> state.root_argp = argp;
> _help (argp, , stream, flags, name);
> }
>
> As a result it was possible at the time to encounter the case where
> state was non-NULL but state->pstate was NULL. However, this seems to
> have been fixed differently some time ago:
>
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=14a582531c
>
> So I think it's probably fine to drop this patch from GRUB.
Appreciate the follow-up! That sounds reasonable to me.
Be well,
--Robbie
signature.asc
Description: PGP signature
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
Paul Eggert writes:
> On 12/1/21 13:02, Robbie Harwood wrote:
>
>> diff --git a/lib/argp-fmtstream.c b/lib/argp-fmtstream.c
>> index f679751b9..4aa401e2d 100644
>> --- a/lib/argp-fmtstream.c
>> +++ b/lib/argp-fmtstream.c
>> @@ -234,7 +234,7 @@ __argp_fmtstream_update (argp_fmtstream_t fs)
>> else
>> {
>>size_t display_width = mbsnwidth (buf, nl - buf, MBSW_STOP_AT_NUL);
>> - if (display_width < (ssize_t) fs->rmargin)
>> + if (display_width < fs->rmargin)
>> {
>>/* The buffer contains a full line that fits within the maximum
>> line width. Reset point and scan the next line. */
>
> This fixes a problem introduced in PATCH v2 04/10 "Fix width
> computation". Please fix that patch instead.
Willdo, thanks.
> Let's not make this sort of change. We are moving to a coding style that
> prefers signed values, since they allow us to use better runtime checks.
> If -Wsign-compare complains, let's disable -Wsign-compare instead of
> changing carefully-written signed integers to be unsigned.
>
> The remaining changes in this patch also seem to be unnecessary; they're
> put in only to pacify -Wsign-compare, so the solution is to not use
> -Wsign-compare. That's what Coreutils does, for example.
I'm not going to pretend it's the most important flag or anything like
that, but it's odd to me to hear disabling it being the suggested course
of action (especially given it's in the gcc -Wextra set, not some random
flag).
As you point out with coreutils, gnulib taking a position like this on
flags has the knock-on effect that, for our grub, we'll need to do one
of the following:
- Carry a gnulib patch to make the flag work (what we've been doing).
- Change flags in the outer work (i.e., change build options for grub)
- Maintain logic to keep flags gnulib-disliked flags out when building
the gnulib modules
Be well,
--Robbie
signature.asc
Description: PGP signature
Re: [PATCH v2 05/10] Make gnulib's regcomp not abort()
Paul Eggert writes: > On 12/1/21 19:20, Paul Eggert wrote: >> On 12/1/21 13:02, Robbie Harwood wrote: >>> @@ -1099,7 +1099,7 @@ optimize_utf8 (re_dfa_t *dfa) >>> } >>> break; >>> default: >>> - abort (); >>> + break; >>> } >> >> Likewise, it's not clear why this change is needed. The 'abort' should >> not be reachable. >> >> Is the intent to make the code a bit smaller by avoding calls to 'abort'? > > A followup idea: would it help to replace 'abort ()' with 'DEBUG_ASSERT > (false)', or to replace 'if (!X) abort ();' with 'DEBUG_ASSERT (X);'? Unfortunately Vladimir has not so far been responding to gnulib emails, However, I don't believe we have an implementation of abort() that can be called. (We have grub_abort() instead.) If that's the correct reason, then DEBUG_ASSERT would work and I can make that change. Be well, --Robbie signature.asc Description: PGP signature
Re: [PATCH v2 03/10] gnulib/regexec: Resolve unused variable
Paul Eggert writes: > On 12/1/21 13:01, Robbie Harwood 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. > > Could you explain what goes wrong? regexec_internal.h does this: > >> #if defined DEBUG && DEBUG != 0 >> # include >> # define DEBUG_ASSERT(x) assert (x) >> #else >> # define DEBUG_ASSERT(x) assume (x) >> #endif > > so the later 'DEBUG_ASSERT (match_last != -1);' expands to 'assume > (match_last != -1);'. And verify.h defines 'assume(R)' to evaluate R so > match_last is used there (unless you're using Microsoft's compiler in > which case it is equivalent to '__assume (R)'; are you having problems > with Microsoft's compiler?). (Same framing as the previous one - Coverity reported, etc.) I believe this change predates your 79f8ee4e389f8cb1339f8abed9a7d29816e2a2d4 by several months (grub currently starts at d271f868a8df9bbec29049d01e056481b7a1a263) and should be therefore safe for grub to drop if it ever rolls forward. Thanks! Be well, --Robbie signature.asc Description: PGP signature
Re: [PATCH v2 02/10] gnulib/regexec: Fix possible null-dereference
Paul Eggert writes: > On 12/1/21 13:01, Robbie Harwood wrote: >> It appears to be possible that the mctx->state_log field may be NULL, > > I don't see how. re_search_internal sets mctx.state_log to a non-null > value if dfa->has_mb_node, and clean_state_log_if_needed should be > called only if dfa->has_mb_node is true. What am I missing? Having a CID number means this is fixing a Coverity issue. I don't have access to that, so maybe Darren/Daniel can provide more information here. My *guess* is that Coverity has noticed that `mctx->state_log` is checked against NULL in many other places in that file, and was unable to prove to itself that it couldn't be NULL there too. If that's the case, a DEBUG_ASSERT would presumably do the trick better. Be well, --Robbie signature.asc Description: PGP signature
Re: [PATCH v2 01/10] argp-parse.c (__argp_input): Don't crash if pstate is NULL
On Tue, Dec 07, 2021 at 11:59:01AM -0500, Robbie Harwood wrote:
> Bruno Haible writes:
> > I don't think this patch is needed, because:
> >
> > 1) The application cannot construct a 'struct argp_state' by itself, since
> > [1]
> >says that the 'struct argp_state' contains a member 'pstate' that is
> >"Private, for use by the argp implementation.".
> >
> > 2) The only place in the gnulib / glibc code where a 'struct argp_state' is
> >being constructed, is in function parser_init, invoked from 'argp_parse',
> >and there a non-NULL value is assigned.
> >
> > In other words, there is no way, compliant with the documented API, that a
> > NULL pointer can arise as state->pstate.
> >
> > Bruno
> >
> > [1]
> > https://www.gnu.org/software/libc/manual/html_node/Argp-Parsing-State.html
>
> Thanks for taking a look. I don't have more information on this one
> beyond the little that's in the commit, so unless Colin remembers why
> this was needed in 2011, I'll propose grub drop it at some point.
The original problem was:
https://bugs.debian.org/612692
At the time, __argp_help looked like this:
void __argp_help (const struct argp *argp, FILE *stream,
unsigned flags, char *name)
{
struct argp_state state;
memset (, 0, sizeof state);
state.root_argp = argp;
_help (argp, , stream, flags, name);
}
As a result it was possible at the time to encounter the case where
state was non-NULL but state->pstate was NULL. However, this seems to
have been fixed differently some time ago:
https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=14a582531c
So I think it's probably fine to drop this patch from GRUB.
--
Colin Watson (he/him) [cjwat...@ubuntu.com]
Re: [PATCH v2 01/10] argp-parse.c (__argp_input): Don't crash if pstate is NULL
Bruno Haible writes:
> Robbie Harwood wrote:
>> From: Colin Watson
>>
>> [rharw...@redhat.com: tweaked commit message]
>> Signed-off-by: Robbie Harwood
>> ---
>> lib/argp-parse.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/argp-parse.c b/lib/argp-parse.c
>> index 053495ec0..4f1c65d73 100644
>> --- a/lib/argp-parse.c
>> +++ b/lib/argp-parse.c
>> @@ -940,7 +940,7 @@ weak_alias (__argp_parse, argp_parse)
>> void *
>> __argp_input (const struct argp *argp, const struct argp_state *state)
>> {
>> - if (state)
>> + if (state && state->pstate)
>> {
>>struct group *group;
>>struct parser *parser = state->pstate;
>
> I don't think this patch is needed, because:
>
> 1) The application cannot construct a 'struct argp_state' by itself, since [1]
>says that the 'struct argp_state' contains a member 'pstate' that is
>"Private, for use by the argp implementation.".
>
> 2) The only place in the gnulib / glibc code where a 'struct argp_state' is
>being constructed, is in function parser_init, invoked from 'argp_parse',
>and there a non-NULL value is assigned.
>
> In other words, there is no way, compliant with the documented API, that a
> NULL pointer can arise as state->pstate.
>
> Bruno
>
> [1] https://www.gnu.org/software/libc/manual/html_node/Argp-Parsing-State.html
Thanks for taking a look. I don't have more information on this one
beyond the little that's in the commit, so unless Colin remembers why
this was needed in 2011, I'll propose grub drop it at some point.
Be well,
--Robbie
signature.asc
Description: PGP signature
Re: [PATCH v2 01/10] argp-parse.c (__argp_input): Don't crash if pstate is NULL
Robbie Harwood wrote:
> From: Colin Watson
>
> [rharw...@redhat.com: tweaked commit message]
> Signed-off-by: Robbie Harwood
> ---
> lib/argp-parse.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/argp-parse.c b/lib/argp-parse.c
> index 053495ec0..4f1c65d73 100644
> --- a/lib/argp-parse.c
> +++ b/lib/argp-parse.c
> @@ -940,7 +940,7 @@ weak_alias (__argp_parse, argp_parse)
> void *
> __argp_input (const struct argp *argp, const struct argp_state *state)
> {
> - if (state)
> + if (state && state->pstate)
> {
>struct group *group;
>struct parser *parser = state->pstate;
I don't think this patch is needed, because:
1) The application cannot construct a 'struct argp_state' by itself, since [1]
says that the 'struct argp_state' contains a member 'pstate' that is
"Private, for use by the argp implementation.".
2) The only place in the gnulib / glibc code where a 'struct argp_state' is
being constructed, is in function parser_init, invoked from 'argp_parse',
and there a non-NULL value is assigned.
In other words, there is no way, compliant with the documented API, that a
NULL pointer can arise as state->pstate.
Bruno
[1] https://www.gnu.org/software/libc/manual/html_node/Argp-Parsing-State.html
