Re: [PATCH] fatal-signal: silence coverity warning

[Jim Meyering]

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