RE: [PATCH] Fix undefined behaviour in lround,llround.

["Howland Craig D (Craig)" <howland at LGSInnovations dot com>]

Dave:
 
A small suggestion for the macros (not a problem as used now,
but paranoia for future):
-(((amt) < 8 * sizeof(op)) ? (op << amt) : 0)
+(((amt) < 8 * sizeof(op)) ? ((op) << (amt)) : 0)
-(((amt) < 8 * sizeof(op)) ? (op >> amt) : 0)
+(((amt) < 8 * sizeof(op)) ? ((op) >> (amt)) : 0)
 
Changes look good to me, but, yes, they are over-cautious in
applying the macro, as they are only needed in one place each in the
files--the first one.  In the final if (two shifts inside),
20 <= exponent_less_1023 <= 51, so the possible shift amounts are only
0 to 31, not needing the protection.
 
The attached revised patch has changes for both of these for your
consideration.
 
I think that this error is likely in more files (lrint is likely),
as there are many similarites.  (I'll have to take a look later.)
 
Craig



-----Original Message-----
From: newlib-owner@sourceware.org on behalf of Dave Korn
Sent: Fri 7/16/2010 12:13 PM
To: newlib@sourceware.org
Subject: [PATCH] Fix undefined behaviour in lround,llround.
 

    Hi all,

  Back at the start of June we had a report(*) of a bug in the gfortran
nint()
function, which I tracked down to a problem in the underlying
implementation
of lround() in newlib.  The bug occurs here:

>   else if (exponent_less_1023 < (8 * sizeof (long int)) - 1)
>     {
>       if (exponent_less_1023 >= 52)
>         result = ((long int) msw << (exponent_less_1023 - 20)) | (lsw
<< (exponent_less_1023 - 52));
>       else
>         {
>           unsigned int tmp = lsw + (0x80000000 >> (exponent_less_1023
- 20));
>           if (tmp < lsw)
>             ++msw;
>           result = ((long int) msw << (exponent_less_1023 - 20)) |
(tmp >> (52 - exponent_less_1023));
>         }
>     }
>   else

  When exponent_less_1023 is exactly 52, the subexpression "((long int)
msw <<
(exponent_less_1023 - 20))" becomes a left shift by 32.  On systems
where a
long int is 32 bits, that's undefined behaviour; the shift amount must
always
be less than the size of the left-hand operand (n1256, 6.5.7#3), and in
practice, this means that a standard x86 shift opcode is used, which has
the
property of implicitly AND-ing the shift amount with 31; the result of a
shift
by 32 bits thus isn't zero as the code expects, but the original
unshifted
value of msw, which gets OR-d into completely the wrong place and splots
bits
all over the correctly-shifted lsw part of the result.

  In this patch I define a couple of safe shift macros that return zero
and
avoid the undefined behaviour when the shift amount is too great.  I've
actually been slightly over-cautious in applying it a couple of places,
I
think, but this isn't the fast math code and anyway value-range analysis
is a
job for a compiler, which it can do bearing in mind the actual type
sizes in
effect during compilation.

newlib/ChangeLog:

	* libm/common/fdlibm.h (SAFE_LEFT_SHIFT): New macro definition.
	(SAFE_RIGHT_SHIFT): Likewise.
	* libm/common/s_llround.c (llround): Use them to avoid undefined
	behaviour.
	* libm/common/s_lround.c (lround): Likewise.

  OK?

    cheers,
      DaveK
-- 
(*) - http://cygwin.com/ml/cygwin/2010-06/threads.html#00315

Attachment: fpmath-lround-fix2.diff
Description: fpmath-lround-fix2.diff