[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] fatal-signal: silence coverity warning
From: |
Pádraig Brady |
Subject: |
Re: [PATCH] fatal-signal: silence coverity warning |
Date: |
Sat, 30 Apr 2011 00:18:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.8) Gecko/20100227 Thunderbird/3.0.3 |
On 29/04/11 22:15, Pádraig Brady wrote:
> On 29/04/11 22:00, Jim Meyering wrote:
>> 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.
>
> Well {0,} is already used in gnulib.
>
> I'm going to change manywarnings.m4 so that
> -Wno-missing-field-initializers is added if needed.
>
> Then we don't need to worry about any of this.
Proposed patch attached.
cheers,
Pádraig.
no-missing-field-initializers.diff
Description: Text Data
- [PATCH] fatal-signal: silence coverity warning, Eric Blake, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning, Jim Meyering, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning, Pádraig Brady, 2011/04/29
- Re: [PATCH] fatal-signal: silence coverity warning,
Pádraig Brady <=
- 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