Use of initialized variable in strtod.c

Use of initialized variable in strtod.c

[Joel Sherrill]

Hi

I have looked at this one enough to believe it is
a real issue but have no idea what the proper
solution is. Maybe someone is more familiar with
this routine can help.

This looks like it was introduced in 2006 by Jeff
Johnston but I don't expect him to remember it. :)

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;f=newlib/libc/stdlib/strtod.c;h=f489b5943c8f8655b0a3caddd38114111576ab35

2006-06-22  Jeff Johnston  <jjohnstn@redhat.com>

         * libc/stdlib/Makefile.am: Add new gdtoa routines.
         * libc/stdlib/Makefile.in: Regenerated.
         * libc/stdlib/gd_qnan.h: New file.
         * libc/stdlib/gdtoa-gethex.c: Ditto.
         * libc/stdlib/gdtoa-hexnan.c: Ditto.
         * libc/stdlib/gdtoa.h: Ditto.
         * libc/stdlib/mprec.c: Add new helper routines needed by
         the new gdtoa code.
         * libc/stdlib/mprec.h: Integrate some defines and prototypes
         used by gdtoa routines here.
         * libc/stdlib/strtod.c: Rebased on David M. Gay's gdtoa-strtod.c
         which adds C99 support such as nan, inf, and hexadecimal input
         format.

Basically if (bb) is false, then bits is not set
and it is used as input to ULtod.

334                                if (bb) {
335                                        copybits(bits, fpi.nbits, bb);
336                                        Bfree(ptr,bb);
337                                        }
    	
CID 175379 (#1 of 1): Uninitialized scalar variable (UNINIT)
10. uninit_use_in_call: Using uninitialized element of array bits when calling ULtod. [show details]
338                                ULtod(rv.i, bits, exp, i);

Hopefully someone has some insight on how to fix this.

Thanks.

-- 
Joel Sherrill, Ph.D.             Director of Research & Development
joel.sherrill@OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35806
Support Available                (256) 722-9985

Re: Use of initialized variable in strtod.c

[Craig Howland]

On 03/15/2017 02:16 PM, Joel Sherrill wrote:
> Hi
>
> I have looked at this one enough to believe it is
> a real issue but have no idea what the proper
> solution is. Maybe someone is more familiar with
> this routine can help.
>
> This looks like it was introduced in 2006 by Jeff
> Johnston but I don't expect him to remember it. :)
>
> https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;f=newlib/libc/stdlib/strtod.c;h=f489b5943c8f8655b0a3caddd38114111576ab35 
>
>
> 2006-06-22  Jeff Johnston  <jjohnstn@redhat.com>
>
>         * libc/stdlib/Makefile.am: Add new gdtoa routines.
>         * libc/stdlib/Makefile.in: Regenerated.
>         * libc/stdlib/gd_qnan.h: New file.
>         * libc/stdlib/gdtoa-gethex.c: Ditto.
>         * libc/stdlib/gdtoa-hexnan.c: Ditto.
>         * libc/stdlib/gdtoa.h: Ditto.
>         * libc/stdlib/mprec.c: Add new helper routines needed by
>         the new gdtoa code.
>         * libc/stdlib/mprec.h: Integrate some defines and prototypes
>         used by gdtoa routines here.
>         * libc/stdlib/strtod.c: Rebased on David M. Gay's gdtoa-strtod.c
>         which adds C99 support such as nan, inf, and hexadecimal input
>         format.
>
> Basically if (bb) is false, then bits is not set
> and it is used as input to ULtod.
>
> 334                                if (bb) {
> 335                                        copybits(bits, fpi.nbits, bb);
> 336                                        Bfree(ptr,bb);
> 337                                        }
>
> CID 175379 (#1 of 1): Uninitialized scalar variable (UNINIT)
> 10. uninit_use_in_call: Using uninitialized element of array bits when calling 
> ULtod. [show details]
> 338                                ULtod(rv.i, bits, exp, i);
>
> Hopefully someone has some insight on how to fix this.
>
> Thanks.
>
I took a quick look, and I think (it's been ages since I had to do some editing 
in strtod.c) it is OK.  Specifically, it does appear that bb is only ever 
returned as 0 in a case when ULtod does not need the value of bits.  So while 
Coverity it right that it could be a problem, it is not really.
Craig

Re: Use of initialized variable in strtod.c

[Joel Sherrill]

On 3/15/2017 1:34 PM, Craig Howland wrote:
> On 03/15/2017 02:16 PM, Joel Sherrill wrote:
>> Hi
>>
>> I have looked at this one enough to believe it is
>> a real issue but have no idea what the proper
>> solution is. Maybe someone is more familiar with
>> this routine can help.
>>
>> This looks like it was introduced in 2006 by Jeff
>> Johnston but I don't expect him to remember it. :)
>>
>> https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;f=newlib/libc/stdlib/strtod.c;h=f489b5943c8f8655b0a3caddd38114111576ab35
>>
>>
>> 2006-06-22  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>         * libc/stdlib/Makefile.am: Add new gdtoa routines.
>>         * libc/stdlib/Makefile.in: Regenerated.
>>         * libc/stdlib/gd_qnan.h: New file.
>>         * libc/stdlib/gdtoa-gethex.c: Ditto.
>>         * libc/stdlib/gdtoa-hexnan.c: Ditto.
>>         * libc/stdlib/gdtoa.h: Ditto.
>>         * libc/stdlib/mprec.c: Add new helper routines needed by
>>         the new gdtoa code.
>>         * libc/stdlib/mprec.h: Integrate some defines and prototypes
>>         used by gdtoa routines here.
>>         * libc/stdlib/strtod.c: Rebased on David M. Gay's gdtoa-strtod.c
>>         which adds C99 support such as nan, inf, and hexadecimal input
>>         format.
>>
>> Basically if (bb) is false, then bits is not set
>> and it is used as input to ULtod.
>>
>> 334                                if (bb) {
>> 335                                        copybits(bits, fpi.nbits, bb);
>> 336                                        Bfree(ptr,bb);
>> 337                                        }
>>
>> CID 175379 (#1 of 1): Uninitialized scalar variable (UNINIT)
>> 10. uninit_use_in_call: Using uninitialized element of array bits when calling
>> ULtod. [show details]
>> 338                                ULtod(rv.i, bits, exp, i);
>>
>> Hopefully someone has some insight on how to fix this.
>>
>> Thanks.
>>
> I took a quick look, and I think (it's been ages since I had to do some editing
> in strtod.c) it is OK.  Specifically, it does appear that bb is only ever
> returned as 0 in a case when ULtod does not need the value of bits.  So while
> Coverity it right that it could be a problem, it is not really.

Would it be better to initialize bb to 0? Or assign it on
the else to "if (bb)". If that's correct, it would make
the intent clearer and eliminate the use of an uninitialized
variable.

FWIW I am a firm believer in not marking issues as false
positive. In this case, there really is a use of an
uninitialized variable. So we might as well address that.

--joel

> Craig
>

Re: Use of initialized variable in strtod.c

[Craig Howland]

On 03/15/2017 02:38 PM, Joel Sherrill wrote:
>
>
> On 3/15/2017 1:34 PM, Craig Howland wrote:
>> On 03/15/2017 02:16 PM, Joel Sherrill wrote:
>>> ...
>>>
>>> Basically if (bb) is false, then bits is not set
>>> and it is used as input to ULtod.
>>>
>>> 334                                if (bb) {
>>> 335                                        copybits(bits, fpi.nbits, bb);
>>> 336                                        Bfree(ptr,bb);
>>> 337                                        }
>>>
>>> CID 175379 (#1 of 1): Uninitialized scalar variable (UNINIT)
>>> 10. uninit_use_in_call: Using uninitialized element of array bits when calling
>>> ULtod. [show details]
>>> 338                                ULtod(rv.i, bits, exp, i);
>>>
>> I took a quick look, and I think (it's been ages since I had to do some editing
>> in strtod.c) it is OK.  Specifically, it does appear that bb is only ever
>> returned as 0 in a case when ULtod does not need the value of bits.  So while
>> Coverity it right that it could be a problem, it is not really.
>
> Would it be better to initialize bb to 0? Or assign it on
> the else to "if (bb)". If that's correct, it would make
> the intent clearer and eliminate the use of an uninitialized
> variable.
>
> FWIW I am a firm believer in not marking issues as false
> positive. In this case, there really is a use of an
> uninitialized variable. So we might as well address that.
>
I disagree that there really is use of an uninitialized variable. There is not.  
It just appears to the tool that there is.  (This is a tough case, so it's not a 
surprise that it misses it and gives a false indictment.)

Does Coverity have a way in which in the code it can be marked as OK?  (I'd 
expect some '#pragma CoverityIgnore(bits)' or the like ought to be available.)  
I agree with trying to get rid of the message, but it is worth bloat to do it?  
(It will add instructions to either initialize bits to 0 or add the else.)  I 
would rather mark something in the code as a false positive than add code 
because the tool is not smart enough to know--so we might differ in philosophy 
there.  I do agree that using out-of-band notes to say to ignore a message would 
be undesirable--but still an interesting question against unnecessarily 
increasing code size.  Adding a Coverity pragma would be best, as then the tool 
would automatically get the report right.  (If I added initialization every time 
GCC complained, I'd get noticeably bigger.  Perhaps Coverity does a much better 
job and much less would need to be added to appease it, but I have no experience 
with it to know.)  Perhaps Corinna or Jeff can weigh in to set a policy.

Craig

Re: Use of initialized variable in strtod.c

[Jeffrey Walton]

> Does Coverity have a way in which in the code it can be marked as OK?  (I'd
> expect some '#pragma CoverityIgnore(bits)' or the like ought to be
> available.)

Yes. You have to provide a modeling file. Also see the Coverity Scan
FAQ entry "what is a model" at https://scan.coverity.com/faq.

Other projects use them, like Python. See, for example,
https://docs.python.org/devguide/coverity.html.

> I agree with trying to get rid of the message, but it is worth
> bloat to do it?  (It will add instructions to either initialize bits to 0 or
> add the else.)

If I am parsing things correctly, it seems like the bloat is going the
other way: if the code is not needed, then remove it. It will avoid
findings like these, and speed up the compile.

> I would rather mark something in the code as a false
> positive than add code because the tool is not smart enough to know--so we
> might differ in philosophy there.

Perhaps a better strategy would be to initialize all variables, and
then allow the optimizer to remove the unneeded writes. It will ensure
a program is in a good state, and avoid findings like these.

Another strategy is to do nothing. In this case, the same findings
will waste multiple developer's time, and generate additional mailing
list messages.

I like dark and silent cockpits, so I don't want tools generating
findings, and I don't want mailing list messages. I would squash it
once and for all and avoid all future problems. But that's just me,
and I understand the Newlib project may have a different outlook on
things.

Jeff

Re: Use of initialized variable in strtod.c

[Joel Sherrill]

On 3/15/2017 2:31 PM, Jeffrey Walton wrote:
>> Does Coverity have a way in which in the code it can be marked as OK?  (I'd
>> expect some '#pragma CoverityIgnore(bits)' or the like ought to be
>> available.)
>
> Yes. You have to provide a modeling file. Also see the Coverity Scan
> FAQ entry "what is a model" at https://scan.coverity.com/faq.

A model is for odd cases like where you have your
own memory allocators or synchronization primitives.

I think this is a case for what they call annotations.
I have never written one of these but I think we would
have to add a comment something like this ahead of the
call:

/* coverity[uninit_use_in_call] */

I will try adding that notation but we clearly need
some guidelines as a project.

> Other projects use them, like Python. See, for example,
> https://docs.python.org/devguide/coverity.html.
>
>> I agree with trying to get rid of the message, but it is worth
>> bloat to do it?  (It will add instructions to either initialize bits to 0 or
>> add the else.)
>
> If I am parsing things correctly, it seems like the bloat is going the
> other way: if the code is not needed, then remove it. It will avoid
> findings like these, and speed up the compile.
>
>> I would rather mark something in the code as a false
>> positive than add code because the tool is not smart enough to know--so we
>> might differ in philosophy there.
>
> Perhaps a better strategy would be to initialize all variables, and
> then allow the optimizer to remove the unneeded writes. It will ensure
> a program is in a good state, and avoid findings like these.

I'm a middle of the road guy. I add initialization in cases
where there are paths where it is used and doesn't otherwise
get set. I wouldn't automatically initialize everything.

In this case, "bits" is actually used on the RHS multiple
times in ULtod() so it bothers me that it has an undefined
value. That means the output of ULtod() is undefined in
this case.
  
> Another strategy is to do nothing. In this case, the same findings
> will waste multiple developer's time, and generate additional mailing
> list messages.

Agreed.
  
> I like dark and silent cockpits, so I don't want tools generating
> findings, and I don't want mailing list messages. I would squash it
> once and for all and avoid all future problems. But that's just me,
> and I understand the Newlib project may have a different outlook on
> things.

+1

There are still 60 others issues. We should do our best to squash
them permanently. IMO marking them with Coverity specific annotation
just means that another static analyzer may find the same issue
in the future. The annotation won't help.
  
> Jeff
>
--joel

Re: Use of initialized variable in strtod.c

[Joel Sherrill]

On 3/15/2017 2:54 PM, Joel Sherrill wrote:
>
>
> On 3/15/2017 2:31 PM, Jeffrey Walton wrote:
>>> Does Coverity have a way in which in the code it can be marked as OK?  (I'd
>>> expect some '#pragma CoverityIgnore(bits)' or the like ought to be
>>> available.)
>>
>> Yes. You have to provide a modeling file. Also see the Coverity Scan
>> FAQ entry "what is a model" at https://scan.coverity.com/faq.
>
> A model is for odd cases like where you have your
> own memory allocators or synchronization primitives.
>
> I think this is a case for what they call annotations.
> I have never written one of these but I think we would
> have to add a comment something like this ahead of the
> call:
>
> /* coverity[uninit_use_in_call] */

Adding this does result in silencing Coverity on this issue.
It doesn't change the fact that the uninitialized bits
variable is used on the RHS of ULtod() though. :(

> I will try adding that notation but we clearly need
> some guidelines as a project.
>
>> Other projects use them, like Python. See, for example,
>> https://docs.python.org/devguide/coverity.html.
>>
>>> I agree with trying to get rid of the message, but it is worth
>>> bloat to do it?  (It will add instructions to either initialize bits to 0 or
>>> add the else.)
>>
>> If I am parsing things correctly, it seems like the bloat is going the
>> other way: if the code is not needed, then remove it. It will avoid
>> findings like these, and speed up the compile.
>>
>>> I would rather mark something in the code as a false
>>> positive than add code because the tool is not smart enough to know--so we
>>> might differ in philosophy there.
>>
>> Perhaps a better strategy would be to initialize all variables, and
>> then allow the optimizer to remove the unneeded writes. It will ensure
>> a program is in a good state, and avoid findings like these.
>
> I'm a middle of the road guy. I add initialization in cases
> where there are paths where it is used and doesn't otherwise
> get set. I wouldn't automatically initialize everything.
>
> In this case, "bits" is actually used on the RHS multiple
> times in ULtod() so it bothers me that it has an undefined
> value. That means the output of ULtod() is undefined in
> this case.
>
>> Another strategy is to do nothing. In this case, the same findings
>> will waste multiple developer's time, and generate additional mailing
>> list messages.
>
> Agreed.
>
>> I like dark and silent cockpits, so I don't want tools generating
>> findings, and I don't want mailing list messages. I would squash it
>> once and for all and avoid all future problems. But that's just me,
>> and I understand the Newlib project may have a different outlook on
>> things.
>
> +1
>
> There are still 60 others issues. We should do our best to squash
> them permanently. IMO marking them with Coverity specific annotation
> just means that another static analyzer may find the same issue
> in the future. The annotation won't help.
>
>> Jeff
>>
> --joel
>

Re: Use of initialized variable in strtod.c

[Joel Sherrill]

Sorry to keep replying to myself. More below.

On 3/15/2017 3:03 PM, Joel Sherrill wrote:
>
>
> On 3/15/2017 2:54 PM, Joel Sherrill wrote:
>>
>>
>> On 3/15/2017 2:31 PM, Jeffrey Walton wrote:
>>>> Does Coverity have a way in which in the code it can be marked as OK?  (I'd
>>>> expect some '#pragma CoverityIgnore(bits)' or the like ought to be
>>>> available.)
>>>
>>> Yes. You have to provide a modeling file. Also see the Coverity Scan
>>> FAQ entry "what is a model" at https://scan.coverity.com/faq.
>>
>> A model is for odd cases like where you have your
>> own memory allocators or synchronization primitives.
>>
>> I think this is a case for what they call annotations.
>> I have never written one of these but I think we would
>> have to add a comment something like this ahead of the
>> call:
>>
>> /* coverity[uninit_use_in_call] */
>
> Adding this does result in silencing Coverity on this issue.
> It doesn't change the fact that the uninitialized bits
> variable is used on the RHS of ULtod() though. :(

But this in __call_atexit.c is definitely correct. It is
treating free() as a weak symbol and the only way to
silence Coverity is to add an annotation.

136      /* Don't dynamically free the atexit array if free is not
137         available.  */
    	
CID 175323 (#1 of 1): Function address comparison (BAD_COMPARE)
func_conv: This implicit conversion to a function pointer is suspicious: free.
    	Did you intend to call free?
138      if (!free)
139        break;
  
>> I will try adding that notation but we clearly need
>> some guidelines as a project.

Agreed. There appear to be cases where annotation is the
only solution. Is adding annotation acceptable?

If a solution other than annotation exists, is that
the preferred option?

>>> Other projects use them, like Python. See, for example,
>>> https://docs.python.org/devguide/coverity.html.
>>>
>>>> I agree with trying to get rid of the message, but it is worth
>>>> bloat to do it?  (It will add instructions to either initialize bits to 0 or
>>>> add the else.)
>>>
>>> If I am parsing things correctly, it seems like the bloat is going the
>>> other way: if the code is not needed, then remove it. It will avoid
>>> findings like these, and speed up the compile.
>>>
>>>> I would rather mark something in the code as a false
>>>> positive than add code because the tool is not smart enough to know--so we
>>>> might differ in philosophy there.
>>>
>>> Perhaps a better strategy would be to initialize all variables, and
>>> then allow the optimizer to remove the unneeded writes. It will ensure
>>> a program is in a good state, and avoid findings like these.
>>
>> I'm a middle of the road guy. I add initialization in cases
>> where there are paths where it is used and doesn't otherwise
>> get set. I wouldn't automatically initialize everything.
>>
>> In this case, "bits" is actually used on the RHS multiple
>> times in ULtod() so it bothers me that it has an undefined
>> value. That means the output of ULtod() is undefined in
>> this case.
>>
>>> Another strategy is to do nothing. In this case, the same findings
>>> will waste multiple developer's time, and generate additional mailing
>>> list messages.
>>
>> Agreed.
>>
>>> I like dark and silent cockpits, so I don't want tools generating
>>> findings, and I don't want mailing list messages. I would squash it
>>> once and for all and avoid all future problems. But that's just me,
>>> and I understand the Newlib project may have a different outlook on
>>> things.
>>
>> +1
>>
>> There are still 60 others issues. We should do our best to squash
>> them permanently. IMO marking them with Coverity specific annotation
>> just means that another static analyzer may find the same issue
>> in the future. The annotation won't help.
>>
>>> Jeff
>>>
>> --joel
>>
>

-- 
Joel Sherrill, Ph.D.             Director of Research & Development
joel.sherrill@OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35806
Support Available                (256) 722-9985

Re: Use of initialized variable in strtod.c

[Craig Howland]

On 03/15/2017 06:36 PM, Joel Sherrill wrote:
> But this in __call_atexit.c is definitely correct. It is
> treating free() as a weak symbol and the only way to
> silence Coverity is to add an annotation.
>
> 136      /* Don't dynamically free the atexit array if free is not
> 137         available.  */
>
> CID 175323 (#1 of 1): Function address comparison (BAD_COMPARE)
> func_conv: This implicit conversion to a function pointer is suspicious: free.
>        Did you intend to call free?
> 138      if (!free)
> 139        break; 
This should be able to be gotten rid of easily enough.  (I don't see why the 
"weak" aspect would have anything to do with it.)

For example:
if(free == NULL)
if(!(uintptr_t)free)

Craig

Re: Use of initialized variable in strtod.c

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

On Mar 15 14:56, Craig Howland wrote:
> On 03/15/2017 02:38 PM, Joel Sherrill wrote:
> > 
> > 
> > On 3/15/2017 1:34 PM, Craig Howland wrote:
> > > On 03/15/2017 02:16 PM, Joel Sherrill wrote:
> > > > ...
> > > > 
> > > > Basically if (bb) is false, then bits is not set
> > > > and it is used as input to ULtod.
> > > > 
> > > > 334                                if (bb) {
> > > > 335                                        copybits(bits, fpi.nbits, bb);
> > > > 336                                        Bfree(ptr,bb);
> > > > 337                                        }
> > > > 
> > > > CID 175379 (#1 of 1): Uninitialized scalar variable (UNINIT)
> > > > 10. uninit_use_in_call: Using uninitialized element of array bits when calling
> > > > ULtod. [show details]
> > > > 338                                ULtod(rv.i, bits, exp, i);
> > > > 
> > > I took a quick look, and I think (it's been ages since I had to do some editing
> > > in strtod.c) it is OK.  Specifically, it does appear that bb is only ever
> > > returned as 0 in a case when ULtod does not need the value of bits.  So while
> > > Coverity it right that it could be a problem, it is not really.
> > 
> > Would it be better to initialize bb to 0? Or assign it on
> > the else to "if (bb)". If that's correct, it would make
> > the intent clearer and eliminate the use of an uninitialized
> > variable.
> > 
> > FWIW I am a firm believer in not marking issues as false
> > positive. In this case, there really is a use of an
> > uninitialized variable. So we might as well address that.
> > 
> I disagree that there really is use of an uninitialized variable. There is
> not.  It just appears to the tool that there is.  (This is a tough case, so
> it's not a surprise that it misses it and gives a false indictment.)
> 
> Does Coverity have a way in which in the code it can be marked as OK?  (I'd
> expect some '#pragma CoverityIgnore(bits)' or the like ought to be
> available.)  I agree with trying to get rid of the message, but it is worth
> bloat to do it?

Just mark it as false positive in coverity.  We should not change the
code in this area too much.  It's basically David M. Gay's gdtoa code
and we should keep it in a shape easier comparable with upstream.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]