SF bug 705836: struct.pack of floats in non-native endian order

pack_float, pack_double, save_float:  All the routines for creating
IEEE-format packed representations of floats and doubles simply ignored
that rounding can (in rare cases) propagate out of a long string of
1 bits.  At worst, the end-off carry can (by mistake) interfere with
the exponent value, and then unpacking yields a result wrong by a factor
of 2.  In less severe cases, it can end up losing more low-order bits
than intended, or fail to catch overflow *caused* by rounding.

diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py
index 34abad55ea267cfb485903abef5cce0319a3c074..a5d91a2cc3890cda1b2c8e701f0671ef9fde0582 100644 (file)
--- a/Lib/test/test_struct.py
+++ b/Lib/test/test_struct.py
@@ -393,3 +393,49 @@ def test_p_code():
                              (code, input, got, expectedback))
 
 test_p_code()
+
+
+###########################################################################
+# SF bug 705836.  "<f" and ">f" had a severe rounding bug, where a carry
+# from the low-order discarded bits could propagate into the exponent
+# field, causing the result to be wrong by a factor of 2.
+
+def test_705836():
+    import math
+
+    for base in range(1, 33):
+        # smaller <- largest representable float less than base.
+        delta = 0.5
+        while base - delta / 2.0 != base:
+            delta /= 2.0
+        smaller = base - delta
+        # Packing this rounds away a solid string of trailing 1 bits.
+        packed = struct.pack("<f", smaller)
+        unpacked = struct.unpack("<f", packed)[0]
+        # This failed at base = 2, 4, and 32, with unpacked = 1, 2, and
+        # 16, respectively.
+        verify(base == unpacked)
+        bigpacked = struct.pack(">f", smaller)
+        verify(bigpacked == string_reverse(packed),
+               ">f pack should be byte-reversal of <f pack")
+        unpacked = struct.unpack(">f", bigpacked)[0]
+        verify(base == unpacked)
+
+    # Largest finite IEEE single.
+    big = (1 << 24) - 1
+    big = math.ldexp(big, 127 - 23)
+    packed = struct.pack(">f", big)
+    unpacked = struct.unpack(">f", packed)[0]
+    verify(big == unpacked)
+
+    # The same, but tack on a 1 bit so it rounds up to infinity.
+    big = (1 << 25) - 1
+    big = math.ldexp(big, 127 - 24)
+    try:
+        packed = struct.pack(">f", big)
+    except OverflowError:
+        pass
+    else:
+        TestFailed("expected OverflowError")
+
+test_705836()

diff --git a/Misc/NEWS b/Misc/NEWS
index 9335010e86d557aed2241a747e515384762b3ab0..56f0a80384b5d926e36077d887f9baaad251e92e 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -2,6 +2,14 @@ What's New in Python 2.2.3 ?
 Release date: XX-XXX-2003
 ============================
 
+- SF #705836: The platform-independent routines for packing floats in
+  IEEE formats (struct.pack's <f, >f, <d, and >d codes; pickle and
+  cPickle's protocol 1 pickling of floats) ignored that rounding can
+  cause a carry to propagate.  The worst consequence was that, in rare
+  cases, <f and >f could produce strings that, when unpacked again,
+  were a factor of 2 away from the original float.  This has been
+  fixed.
+
 - Backported SF patch #676342: after using pdb, the readline command
   completion was botched.
 
@@ -31,7 +39,7 @@ Release date: XX-XXX-2003
   value, but according to PEP 237 it really needs to be 1 in Python
   2.2.3 and 2.3.  (SF #660455)
 
-- SF bug #678518:  fix some bugs in the parser module.  
+- SF bug #678518:  fix some bugs in the parser module.
 
 - Bastion.py and rexec.py are disabled.  These modules are not safe in
   Python 2.2. or 2.3.
@@ -103,7 +111,7 @@ Release date: XX-XXX-2003
 
 - SF #570655, fix misleading option text for bdist_rpm
 
-- Distutils: Allow unknown keyword arguments to the setup() function 
+- Distutils: Allow unknown keyword arguments to the setup() function
   and the Extension constructor, printing a warning about them instead
   of reporting an error and stopping.
 
@@ -419,7 +427,7 @@ Library
     gauss() may see different results now.
 
   - The randint() method is rehabilitated (i.e. no longer deprecated).
-  
+
 -  In copy.py: when an object is copied through its __reduce__ method,
    there was no check for a __setstate__ method on the result [SF
    patch 565085]; deepcopy should treat instances of custom

diff --git a/Modules/cPickle.c b/Modules/cPickle.c
index ae2df8c21af10d3333cdd63b89efbf823fcc116f..214dc62ccd4feea340751a3d7bae0e64617124dc 100644 (file)
--- a/Modules/cPickle.c
+++ b/Modules/cPickle.c
@@ -705,7 +705,7 @@ get(Picklerobject *self, PyObject *id) {
 
 static int
 put(Picklerobject *self, PyObject *ob) {
-    if (ob->ob_refcnt < 2 || self->fast) 
+    if (ob->ob_refcnt < 2 || self->fast)
         return 0;
 
     return put2(self, ob);
@@ -921,7 +921,7 @@ fast_save_enter(Picklerobject *self, PyObject *obj)
     return 1;
 }
 
-int 
+int
 fast_save_leave(Picklerobject *self, PyObject *obj)
 {
     if (self->fast_container-- >= PY_CPICKLE_FAST_LIMIT) {
@@ -1064,12 +1064,8 @@ save_float(Picklerobject *self, PyObject *args) {
             return -1;
         }
 
-        if (e >= 1024) {
-            /* XXX 1024 itself is reserved for Inf/NaN */
-            PyErr_SetString(PyExc_OverflowError,
-                            "float too large to pack with d format");
-            return -1;
-        }
+        if (e >= 1024)
+           goto Overflow;
         else if (e < -1022) {
             /* Gradual underflow */
             f = ldexp(f, 1022 + e);
@@ -1083,9 +1079,26 @@ save_float(Picklerobject *self, PyObject *args) {
         /* fhi receives the high 28 bits; flo the low 24 bits (== 52 bits) */
         f *= 268435456.0; /* 2**28 */
         fhi = (long) floor(f); /* Truncate */
+       assert(fhi < 268435456);
+
         f -= (double)fhi;
         f *= 16777216.0; /* 2**24 */
         flo = (long) floor(f + 0.5); /* Round */
+       assert(flo <= 16777216);
+       if (flo >> 24) {
+               /* The carry propagated out of a string of 24 1 bits. */
+               flo = 0;
+               ++fhi;
+               if (fhi >> 28) {
+                       /* And it also progagated out of the next
+                        * 28 bits.
+                        */
+                       fhi = 0;
+                       ++e;
+                       if (e >= 2047)
+                               goto Overflow;
+               }
+       }
 
         /* First byte */
         *p = (s<<7) | (e>>4);
@@ -1131,6 +1144,11 @@ save_float(Picklerobject *self, PyObject *args) {
     }
 
     return 0;
+
+ Overflow:
+       PyErr_SetString(PyExc_OverflowError,
+                       "float too large to pack with d format");
+       return -1;
 }
 
 
@@ -2103,9 +2121,9 @@ dump(Picklerobject *self, PyObject *args) {
 
 static PyObject *
 Pickle_clear_memo(Picklerobject *self, PyObject *args) {
-    if (!PyArg_ParseTuple(args,":clear_memo")) 
+    if (!PyArg_ParseTuple(args,":clear_memo"))
        return NULL;
-    if (self->memo) 
+    if (self->memo)
        PyDict_Clear(self->memo);
     Py_INCREF(Py_None);
     return Py_None;
@@ -2120,7 +2138,7 @@ Pickle_getvalue(Picklerobject *self, PyObject *args) {
   Pdata *data;
 
   /* Can be called by Python code or C code */
-  if (args && !PyArg_ParseTuple(args, "|i:getvalue", &clear)) 
+  if (args && !PyArg_ParseTuple(args, "|i:getvalue", &clear))
       return NULL;
 
   /* Check to make sure we are based on a list */
@@ -2482,7 +2500,7 @@ static PyMemberDef Pickler_members[] = {
 };
 
 static PyGetSetDef Pickler_getsets[] = {
-    {"persistent_id", (getter)Pickler_get_pers_func, 
+    {"persistent_id", (getter)Pickler_get_pers_func,
                      (setter)Pickler_set_pers_func},
     {"inst_persistent_id", NULL, (setter)Pickler_set_inst_pers_func},
     {"memo", (getter)Pickler_get_memo, (setter)Pickler_set_memo},

diff --git a/Modules/structmodule.c b/Modules/structmodule.c
index b76e311bc8f045fa204a4b715c9acca0311c8089..c2b0f2143469d83d359ccae859999b9ad746403a 100644 (file)
--- a/Modules/structmodule.c
+++ b/Modules/structmodule.c
@@ -225,12 +225,8 @@ pack_float(double x, /* The number to pack */
                return -1;
        }
 
-       if (e >= 128) {
-               /* XXX 128 itself is reserved for Inf/NaN */
-               PyErr_SetString(PyExc_OverflowError,
-                               "float too large to pack with f format");
-               return -1;
-       }
+       if (e >= 128)
+               goto Overflow;
        else if (e < -126) {
                /* Gradual underflow */
                f = ldexp(f, 126 + e);
@@ -243,6 +239,14 @@ pack_float(double x, /* The number to pack */
 
        f *= 8388608.0; /* 2**23 */
        fbits = (long) floor(f + 0.5); /* Round */
+       assert(fbits <= 8388608);
+       if (fbits >> 23) {
+               /* The carry propagated out of a string of 23 1 bits. */
+               fbits = 0;
+               ++e;
+               if (e >= 255)
+                       goto Overflow;
+       }
 
        /* First byte */
        *p = (s<<7) | (e>>1);
@@ -261,6 +265,11 @@ pack_float(double x, /* The number to pack */
 
        /* Done */
        return 0;
+
+ Overflow:
+       PyErr_SetString(PyExc_OverflowError,
+                       "float too large to pack with f format");
+       return -1;
 }
 
 static int
@@ -296,12 +305,8 @@ pack_double(double x, /* The number to pack */
                return -1;
        }
 
-       if (e >= 1024) {
-               /* XXX 1024 itself is reserved for Inf/NaN */
-               PyErr_SetString(PyExc_OverflowError,
-                               "float too large to pack with d format");
-               return -1;
-       }
+       if (e >= 1024)
+               goto Overflow;
        else if (e < -1022) {
                /* Gradual underflow */
                f = ldexp(f, 1022 + e);
@@ -315,9 +320,24 @@ pack_double(double x, /* The number to pack */
        /* fhi receives the high 28 bits; flo the low 24 bits (== 52 bits) */
        f *= 268435456.0; /* 2**28 */
        fhi = (long) floor(f); /* Truncate */
+       assert(fhi < 268435456);
+
        f -= (double)fhi;
        f *= 16777216.0; /* 2**24 */
        flo = (long) floor(f + 0.5); /* Round */
+       assert(flo <= 16777216);
+       if (flo >> 24) {
+               /* The carry propagated out of a string of 24 1 bits. */
+               flo = 0;
+               ++fhi;
+               if (fhi >> 28) {
+                       /* And it also progagated out of the next 28 bits. */
+                       fhi = 0;
+                       ++e;
+                       if (e >= 2047)
+                               goto Overflow;
+               }
+       }
 
        /* First byte */
        *p = (s<<7) | (e>>4);
@@ -353,6 +373,11 @@ pack_double(double x, /* The number to pack */
 
        /* Done */
        return 0;
+
+ Overflow:
+       PyErr_SetString(PyExc_OverflowError,
+                       "float too large to pack with d format");
+       return -1;
 }
 
 static PyObject *