[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] fatal-signal: silence coverity warning
From: |
Jim Meyering |
Subject: |
Re: [PATCH] fatal-signal: silence coverity warning |
Date: |
Fri, 29 Apr 2011 23:00:03 +0200 |
Eric Blake wrote:
> On a glibc system, Coverity gives this warning:
>
> Error: UNINIT:
> m4-1.4.16/lib/fatal-signal.c:183: var_decl: Declaring variable "action"
> without initializer.
> m4-1.4.16/lib/fatal-signal.c:198: uninit_use_in_call: Using uninitialized
> value "action": field "action".sa_restorer is uninitialized when calling
> "sigaction".
>
> For glibc, the warning is spurious (the sa_restorer field is
> silently overwritten inside the sigaction() implementation, so
> it doesn't matter what the user assigns there, and leaving it
> unitialized in the user is actually slightly more efficient).
> But it could very well be a valid bug report for other platforms,
> since POSIX allows other extension fields in struct sigaction;
> always setting all extension fields to 0 (via memset) will
> guarantee that those extension fields can't have arbitrary
> behavior due to being uninitialized.
>
> * lib/fatal-signal.c (install_handlers): Clear undocumented fields.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>
> I'm not sure whether we should apply this patch. On the one
> hand, you can argue (as I did in the commit message) that
> uninitialized hidden members can be dangerous. On the other
> hand, you can argue that if you stick to just the POSIX-defined
> sa_flags values, then you can't trigger any extensions that
> would care about the contents of extension fields in the
> first palce.
>
> Thoughts on whether I should apply or ditch this patch?
...
> diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
> index aca9027..80ffda5 100644
> --- a/lib/fatal-signal.c
> +++ b/lib/fatal-signal.c
> @@ -182,6 +182,7 @@ install_handlers ()
> size_t i;
> struct sigaction action;
>
> + memset (&action, 0, sizeof action);
> action.sa_handler = &fatal_signal_handler;
> /* If we get a fatal signal while executing fatal_signal_handler, enter
> fatal_signal_handler recursively, since it is reentrant. Hence no
I think the case for clearing the bits is a little
stronger than the one for leaving them uninitialized, and would
be even more in favor, it if only this initialization were portable:
struct sigaction action = {0,};
Now that gcc is fixed,
maybe we should use something like DECLARE_ZEROED_AUTO,
http://thread.gmane.org/gmane.comp.gnu.coreutils.general/1124/focus=1131
here in gnulib, too.
FYI, from coreutils/src/system.h:
/* With -Dlint, avoid warnings from gcc about code like mbstate_t m = {0,};
by wasting space on a static variable of the same type, that is thus
guaranteed to be initialized to 0, and use that on the RHS. */
#define DZA_CONCAT0(x,y) x ## y
#define DZA_CONCAT(x,y) DZA_CONCAT0 (x, y)
#ifdef lint
# define DECLARE_ZEROED_AGGREGATE(Type, Var) \
static Type DZA_CONCAT (s0_, __LINE__); Type Var = DZA_CONCAT (s0_,
__LINE__)
#else
# define DECLARE_ZEROED_AGGREGATE(Type, Var) \
Type Var = { 0, }
#endif
- [PATCH] fatal-signal: silence coverity warning, Eric Blake, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning,
Jim Meyering <=
- Re: [PATCH] fatal-signal: silence coverity warning, Pádraig Brady, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning, Pádraig Brady, 2011/04/30
- Re: [PATCH] fatal-signal: silence coverity warning, Eric Blake, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning, Jim Meyering, 2011/04/30
- Re: [PATCH] fatal-signal: silence coverity warning, Pádraig Brady, 2011/04/30
- Re: manywarnings.m4 indentation, Bruno Haible, 2011/04/30
- Re: manywarnings.m4 indentation, Pádraig Brady, 2011/04/30
- Re: [PATCH] fatal-signal: silence coverity warning, Bruno Haible, 2011/04/30
Re: [PATCH] fatal-signal: silence coverity warning, Eric Blake, 2011/04/29
Re: [PATCH] fatal-signal: silence coverity warning, Bruno Haible, 2011/04/29