[SSSD] [PATCH] MONITOR: Fix warning may be used uninitialized

Michal Židek mzidek at redhat.com
Fri Oct 24 18:30:09 UTC 2014


On 10/24/2014 07:13 PM, Jakub Hrozek wrote:
> On Thu, Oct 23, 2014 at 08:11:00PM +0200, Lukas Slebodnik wrote:
>> On (23/10/14 19:26), Michal Židek wrote:
>>> On 10/23/2014 07:09 PM, Jakub Hrozek wrote:
>>>> On Thu, Oct 23, 2014 at 07:02:56PM +0200, Lukas Slebodnik wrote:
>>>>> On (23/10/14 18:38), Jakub Hrozek wrote:
>>>>>> On Thu, Oct 23, 2014 at 06:35:07PM +0200, Jakub Hrozek wrote:
>>>>>>> On Thu, Oct 23, 2014 at 02:14:03PM +0200, Michal Židek wrote:
>>>>>>>> On 10/23/2014 11:36 AM, Lukas Slebodnik wrote:
>>>>>>>>> ehlo,
>>>>>>>>>
>>>>>>>>> src/monitor/monitor.c: In function ‘main’:
>>>>>>>>> src/monitor/monitor.c:2953:24: error: ‘monitor’ may be used uninitialized
>>>>>>>>>                                 in this function [-Werror=maybe-uninitialized]
>>>>>>>>>       monitor->is_daemon = !opt_interactive;
>>>>>>>>>                          ^
>>>>>>>>> cc1: all warnings being treated as errors
>>>>>>>>>
>>>>>>>>> This warning is caused be change 579e5d4b7a3ca161ea7518b2996905fa22c15995
>>>>>>>>> "MONITOR: Allow confdb to be accessed by nonroot user"
>>>>>>>>>
>>>>>>>>> Patch is attached.
>>>>>>>>>
>>>>>>>>> LS
>>>>>>>>>
>>>>>>>
>>>>>>> Not sure I understand, under what circumstances can errno be 0 while
>>>>>>> chown returns 0?
>>>>>>
>>>>>> Err, while chown returns -1
>>>>> Try to explain it gcc with optimization (-O2), but usually gcc does not lie.
>>>>
>>>> I know, but this code just makes wonder what's going on even now, I'm
>>>> pretty sure I will have no idea a year from now...
>>>>
>>>>>
>>>>> another way how to suppress warning is to initialize variabl monitor to NULL
>>>>
>>>> That sounds like a more readable option to me.
>>>
>>> Well... if it really is the case that gcc somehow reorganizes the
>>> optimized code in a way, that errno set by chown is not catched in
>>> the ret variable, then the initialization on NULL will cause crash
>>> (but yes it will suppress the warning). As I said, I think it is
>>> false positive, but I am in favour of the first solution, (the
>>> one I acked already) because it is safer.
>>> o
>
> Is there any way to look at what gcc generates in order to understand
> how we can get to the faulty situation?

We can investigate the assembly code, but I do not think
it is worth the time.

>
>> Michal, Jakub
>> it is your decision. (I don't mind)
>> I just want to get rid of this warning.
>>
>> LS
>
> I'm all for suppressing the warning. But for some reason, the ternary
> operator is not too readable to me..
>
> what about something like:
>
> ret = EINVAL; /* Init ret to suppress gcc warning with high -O level */
> if (errno) ret = errno;
>
> Would that be more readable?

Both ways work for me. The comment in your version is good way
to document why we do this.

>
> Please note I don't like the warning either, I just prefer to only
> upstram code that will be readable even in future when we look for some
> unrelated bug in that area..

Ok. I prepared the patch based on Jakub's suggestion
(with kind approval of Lukas) to speed up the resolution
on this issue.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-MONITOR-Fix-warning-may-be-used-uninitialized.patch
Type: text/x-patch
Size: 1458 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141024/ee1f0926/attachment.bin>


More information about the sssd-devel mailing list