Merge lp:~gz/bzr/2.4_overflow_pack_stat_683191_706957 into lp:bzr/2.4

[Martin Packman]

Truncate stat fields to 4 bytes in pack_stat functions used by dirstate

Fix on 2.4 branch for overflow issues with pack_stat, for a complete discussion of the change see the merge proposal for trunk:
<https://code.launchpad.net/~gz/bzr/overflow_pack_stat_683191_706957/+merge/77372>

There are a couple of things to remark on here in addition.

Firstly, only the version of pack_stat in dirstate that actually gets used in updated, but I haven't removed the old code as was done for trunk in <lp:~gz/bzr/move_pack_stat_to_dirstate_helpers> just in case.

Secondly, the tests here only cover the python implementation, exposing the pyrex version and parametrising against it is only done on trunk. As the code change is identical and the tests work there, this shouldn't matter too much.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 9/28/2011 7:41 PM, Martin Packman wrote:
> Martin Packman has proposed merging
> lp:~gz/bzr/2.4_overflow_pack_stat_683191_706957 into lp:bzr/2.4.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #683191 in
> Bazaar: ""exceptions.OverflowError: long int too large to convert"
> in _dirstate_helpers_pyx.c"
> https://bugs.launchpad.net/bzr/+bug/683191 Bug #706957 in Bazaar:
> "OverflowError in _dirstate_helpers pack_stat"
> https://bugs.launchpad.net/bzr/+bug/706957
>
> For more details, see:
> https://code.launchpad.net/~gz/bzr/2.4_overflow_pack_stat_683191_706957/+merge/77373
>
> Fix on 2.4 branch for overflow issues with pack_stat, for a
> complete discussion of the change see the merge proposal for
> trunk:
> <https://code.launchpad.net/~gz/bzr/overflow_pack_stat_683191_706957/+merge/77372>
>
> There are a couple of things to remark on here in addition.
>
> Firstly, only the version of pack_stat in dirstate that actually
> gets used in updated, but I haven't removed the old code as was
> done for trunk in <lp:~gz/bzr/move_pack_stat_to_dirstate_helpers>
> just in case.
>
> Secondly, the tests here only cover the python implementation,
> exposing the pyrex version and parametrising against it is only
> done on trunk. As the code change is identical and the tests work
> there, this shouldn't matter too much.

 merge: approve

I don't really see why you couldn't copy across everything from the
2.5 implementation, but it isn't a big deal.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6DZ8cACgkQJdeBCYSNAAOx0QCbBkwMc4s+SflCplMbf180INWx
QJUAoJmu03qP9gZsgD60VCV+CMtzrPew
=Azyr
-----END PGP SIGNATURE-----

review: Approve

[Martin Packman (gz)]

> I don't really see why you couldn't copy across everything from the
> 2.5 implementation, but it isn't a big deal.

I moved a bunch of code around on trunk (see the trunk version's prerequisite mp <https://code.launchpad.net/~gz/bzr/move_pack_stat_to_dirstate_helpers/+merge/77013>) which seemed more disruptive than the plain fix here.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/_dirstate_helpers_pyx.pyx'
--- bzrlib/_dirstate_helpers_pyx.pyx	2011-04-22 14:12:22 +0000
+++ bzrlib/_dirstate_helpers_pyx.pyx	2011-09-28 17:40:34 +0000
@@ -97,6 +97,7 @@
     object PyTuple_GetItem_void_object "PyTuple_GET_ITEM" (void* tpl, int index)
     object PyTuple_GET_ITEM(object tpl, Py_ssize_t index)
 
+    unsigned long PyInt_AsUnsignedLongMask(object number) except? -1
 
     char *PyString_AsString(object p)
     char *PyString_AsString_obj "PyString_AsString" (PyObject *string)
@@ -821,12 +822,13 @@
     cdef char result[6*4] # 6 long ints
     cdef int *aliased
     aliased = <int *>result
-    aliased[0] = htonl(stat_value.st_size)
-    aliased[1] = htonl(int(stat_value.st_mtime))
-    aliased[2] = htonl(int(stat_value.st_ctime))
-    aliased[3] = htonl(stat_value.st_dev)
-    aliased[4] = htonl(stat_value.st_ino & 0xFFFFFFFF)
-    aliased[5] = htonl(stat_value.st_mode)
+    aliased[0] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_size))
+    # mtime and ctime will often be floats but get converted to PyInt within
+    aliased[1] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_mtime))
+    aliased[2] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_ctime))
+    aliased[3] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_dev))
+    aliased[4] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_ino))
+    aliased[5] = htonl(PyInt_AsUnsignedLongMask(stat_value.st_mode))
     packed = PyString_FromStringAndSize(result, 6*4)
     return _encode(packed)[:-1]
 

=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py	2011-07-25 07:11:56 +0000
+++ bzrlib/dirstate.py	2011-09-28 17:40:34 +0000
@@ -255,8 +255,9 @@
     # Cannot pre-compile the dirstate pack_stat
     def pack_stat(st, _encode=binascii.b2a_base64, _pack=struct.pack):
         """Convert stat values into a packed representation."""
-        return _encode(_pack('>LLLLLL', st.st_size, int(st.st_mtime),
-            int(st.st_ctime), st.st_dev, st.st_ino & 0xFFFFFFFF,
+        return _encode(_pack('>LLLLLL', st.st_size & 0xFFFFFFFF,
+            int(st.st_mtime) & 0xFFFFFFFF, int(st.st_ctime) & 0xFFFFFFFF,
+            st.st_dev & 0xFFFFFFFF, st.st_ino & 0xFFFFFFFF,
             st.st_mode))[:-1]
 else:
     # compile the struct compiler we need, so as to only do it once

=== modified file 'bzrlib/tests/test__dirstate_helpers.py'
--- bzrlib/tests/test__dirstate_helpers.py	2011-05-06 15:15:44 +0000
+++ bzrlib/tests/test__dirstate_helpers.py	2011-09-28 17:40:34 +0000
@@ -1334,3 +1334,56 @@
         state._sha1_provider = UppercaseSHA1Provider()
         self.assertChangedFileIds(['file-id'], tree)
 
+
+class TestPackStat(tests.TestCase):
+    """Check packed representaton of stat values is robust on all inputs"""
+
+    # GZ 2011-09-26: Should parametrise against all pack_stat implementations
+
+    @staticmethod
+    def pack(statlike_tuple):
+        return dirstate.pack_stat(os.stat_result(statlike_tuple))
+
+    @staticmethod
+    def unpack_field(packed_string, stat_field):
+        return dirstate._unpack_stat(packed_string)[stat_field]
+
+    def test_result(self):
+        self.assertEqual("AAAQAAAAABAAAAARAAAAAgAAAAEAAIHk",
+            self.pack((33252, 1, 2, 0, 0, 0, 4096, 15.5, 16.5, 17.5)))
+
+    def test_giant_inode(self):
+        packed = self.pack((33252, 0xF80000ABC, 0, 0, 0, 0, 0, 0, 0, 0))
+        self.assertEqual(0x80000ABC, self.unpack_field(packed, "st_ino"))
+
+    def test_giant_size(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, (1 << 33) + 4096, 0, 0, 0))
+        self.assertEqual(4096, self.unpack_field(packed, "st_size"))
+
+    def test_fractional_mtime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, 16.9375, 0))
+        self.assertEqual(16, self.unpack_field(packed, "st_mtime"))
+
+    def test_ancient_mtime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, -11644473600.0, 0))
+        self.assertEqual(1240428288, self.unpack_field(packed, "st_mtime"))
+
+    def test_distant_mtime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, 64060588800.0, 0))
+        self.assertEqual(3931046656, self.unpack_field(packed, "st_mtime"))
+
+    def test_fractional_ctime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, 0, 17.5625))
+        self.assertEqual(17, self.unpack_field(packed, "st_ctime"))
+
+    def test_ancient_ctime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, 0, -11644473600.0))
+        self.assertEqual(1240428288, self.unpack_field(packed, "st_ctime"))
+
+    def test_distant_ctime(self):
+        packed = self.pack((33252, 0, 0, 0, 0, 0, 0, 0, 0, 64060588800.0))
+        self.assertEqual(3931046656, self.unpack_field(packed, "st_ctime"))
+
+    def test_negative_dev(self):
+        packed = self.pack((33252, 0, -0xFFFFFCDE, 0, 0, 0, 0, 0, 0, 0))
+        self.assertEqual(0x322, self.unpack_field(packed, "st_dev"))

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-09-26 08:48:20 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-09-28 17:40:34 +0000
@@ -35,6 +35,10 @@
 * Fixed loading of external merge tools from config to properly decode
   command-lines which contain embedded quotes. (Gordon Tyler, #828803)
 
+* Prevent several kinds of OverflowError and other fallout from failing to fit
+  stat fields into four bytes in dirstate pack_stat implementations.
+  (Martin Packman, #683191 #706957)
+
 Documentation
 *************