Skip to content

Commit 1b73b95

Browse files
committed
uniqnum: Remove undefined C behavior
There were several undefined behaviors marked by running ASan on this code; all stemmed from using signed values, where unsigned would work. There was an issue with abs(IV_MIN) not being representable in an IV on a 2's complement machine; and left shifting a negative value is undefined. This commit solves this by converting to use an unsigned value, and stealing the macro now in the perl core to take the abs(IV_MIN) without undefined behavior. I found myself drawing a picture to try to understand the behavior of this. And I think the picture is clearer than the previous text. so I changed to use it. And the intermediate results that were there really just interrupted the flow of the picture, so I removed them.
1 parent bb44050 commit 1b73b95

File tree

1 file changed

+31
-24
lines changed

1 file changed

+31
-24
lines changed

ListUtil.xs

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1495,47 +1495,54 @@ CODE:
14951495
sv_setpvn(keysv, (char *) &nv_arg, ACTUAL_NVSIZE);
14961496
}
14971497
#else /* $Config{nvsize} == $Config{ivsize} == 8 */
1498+
# ifndef NEGATE_2UV /* Assumes iv < 0; avoids undefined behavior if iv == IV_MIN */
1499+
# define NEGATE_2UV(iv) ((UV) -((iv) + 1) + 1U)
1500+
# endif
14981501
if( SvIOK(arg) || !SvOK(arg) ) {
14991502

15001503
/* It doesn't matter if SvUOK(arg) is TRUE */
15011504
IV iv = SvIV(arg);
15021505

15031506
/* use "0" for all zeros */
15041507
if(iv == 0) sv_setpvs(keysv, "0");
1505-
15061508
else {
15071509
int uok = SvUOK(arg);
1508-
int sign = ( iv > 0 || uok ) ? 1 : -1;
1510+
UV uv = (iv > 0 || uok) ? iv : NEGATE_2UV(iv);
15091511

15101512
/* Set keysv to the bytes of SvNV(arg) if and only if the integer value *
15111513
* held by arg can be represented exactly as a double - ie if there are *
15121514
* no more than 51 bits between its least significant set bit and its *
15131515
* most significant set bit. *
15141516
* The neatest approach I could find was provided by roboticus at: *
15151517
* https://www.perlmonks.org/?node_id=11113490 *
1516-
* First, identify the lowest set bit and assign its value to an IV. *
1517-
* Note that this value will always be > 0, and always a power of 2. *
1518-
* *
1518+
* First, identify the lowest set bit. The word will look like *
1519+
* this, with a rightmost set bit in position 's': *
1520+
* ('x's are the values above that bit, and 'y's are their *
1521+
* complements so 'x&y' yields 0) *
1522+
* s *
1523+
* x..xxxxxxxxxx100..00 Original *
1524+
* y..yyyyyyyyyy011..11 Complement *
1525+
* y..yyyyyyyyyy100..00 Add 1 *
1526+
* 0..0000000000100..00 AND with the original *
15191527
* (Yes, complementing and adding 1 is just taking the negative *
1520-
* on 2's complement machines, but not on 1's complement ones) */
1521-
IV lowest_set = iv & (~iv + 1);
1522-
1523-
/* Second, shift it left 53 bits to get location of the first bit *
1524-
* beyond arg's highest "allowed" set bit. *
1525-
* NOTE: If lowest set bit is initially far enough left, then this left *
1526-
* shift operation will result in a value of 0, which is fine. *
1527-
* Then subtract 1 so that all of the ("allowed") bits below the set bit *
1528-
* are 1 && all other ("disallowed") bits are set to 0. *
1529-
* (If the value prior to subtraction was 0, then subtracting 1 will set *
1530-
* all bits - which is also fine.) */
1531-
UV valid_bits = (lowest_set << NV_PRESERVES_UV_BITS) - 1;
1532-
1533-
1534-
/* The value of arg can be exactly represented by a double unless one *
1535-
* or more of its "disallowed" bits are set - ie if iv & (~valid_bits) *
1536-
* is untrue. However, if (iv < 0 && !SvUOK(arg)) we need to multiply iv *
1537-
* by -1 prior to performing that '&' operation - so multiply iv by sign.*/
1538-
if( !((iv * sign) & (~valid_bits)) ) {
1528+
* on 2's complement machines, but not on 1's complement ones, *
1529+
* and some compilers complain about negating an unsigned.) */
1530+
UV lowest_set = uv & (~uv + 1);
1531+
1532+
/* Shift it left by the number of bits in the mantissa, let's *
1533+
* say the mantissa contains 9 bits *
1534+
* |<--9-->| *
1535+
s *
1536+
* 0..0000000000100..00 From above *
1537+
* 0..0100000000000..00 Shift *
1538+
* 0..0011111111111..11 Subtract 1 *
1539+
* 1..1100000000000..00 Complement *
1540+
* x..xx00000000000..00 AND with original *
1541+
* *
1542+
* If the result is 0, all the 'x's were 0, meaning there were *
1543+
* no bits set outside what fits in the mantissa; otherwise it *
1544+
* can't be represented by an NV losslessly. */
1545+
if ((uv & (~ ((lowest_set << NV_PRESERVES_UV_BITS) - 1))) == 0) {
15391546
/* Avoid altering arg's flags */
15401547
nv_arg = uok ? (NV)SvUV(arg) : (NV)SvIV(arg);
15411548
sv_setpvn(keysv, (char *) &nv_arg, 8);

0 commit comments

Comments
 (0)