Undefined Behavior in lib/msun/src/e_pow.c (was Re: New math library from ARM)

[Pedro Giffuni]

On 1/1/19 1:20 AM, Pedro Giffuni wrote:
>
> On 1/1/19 12:52 AM, Steve Kargl wrote:
>> On Tue, Jan 01, 2019 at 12:14:38AM -0500, Pedro Giffuni wrote:
>>>> I'll defer to Bruce on this.  My only comments are
>>>> 1) declarations belong at the top of the file where all 
>>>> declarations occur
>>> Modern standards let you declare variables within blocks. For style we
>>> do prefer to start the function with them but in this case we can't.
>> I'am aware of what modern standards allow.  I'm also
>> aware of the code style of fdlibm, which I think
>> should be maintain.
>
> Well, the alternative is the musl-libc patch, which declares the 
> variables upon use (twice).
>
> I think my patch is a little bit more acceptable.
>
Actually, perhaps a cast is just enough ...

See below ...


>>>> 2) j is already declared as int32_t
>>> And it has to be a signed integer: we later check for negative values.
>> Yeah, I know.  That's why I pointed out the collision.
>>
>>>> 3) uint32_t should be written as u_int32_t.
>>> No, uint32_t is the standard type, u_int32_t is a BSDism. Also, the 
>>> file
>>> consistently uses uint32_t.
>> Ah, no.  e_pow.c uses u_int32_t on line 107.
>
> Duh! I completely misread, sorry about that I probably was still 
> looking at musl-libc instead.
>
> Yes, we should use u_int32_t.
>
>
>>    This would be easy
>> to see if all declarations are grouped together.   In additional,
>> don't you need to include stdint.h to get uint32_t?
>
> Well, my patch did compile,  but I will match the existing style if 
> the change is accepted/desired.
>
>
>> % grep uint32_t /usr/src/lib/msun/src/e_pow.c
>> % grep u_int32_t /usr/src/lib/msun/src/e_pow.c
>>          u_int32_t lx,ly;
>>          n = ((u_int32_t)hx>>31)-1;
>>
>> Code churn to placate lint for an event that cannot occur
>> seems dubious to me.
>>
> UBsan should be able to detect it. I think Undefined Behavior is 
> considered a portability bug.
>
> There is no urgency but it is still a bug.
>
In the new patch: the first j comparison has to be made against an 
unsigned value anyways and the second comparison will never be against 
the magical 31 so there is no need to cast that one.


Pedro.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: msun-pow-ub.diff
Type: text/x-patch
Size: 468 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-numerics/attachments/20190101/e17bf833/attachment.bin>