Merge pull request python#10 from darkk/master

jonathanunderwood · web-flow · commit 88b4f283d54a · 2016-12-23T16:57:20.000Z
Fix lz4.frame.decompress crash caused by incorrect realloc() usage
diff --git a/lz4/block/_block.c b/lz4/block/_block.c
@@ -283,6 +283,12 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * kwargs)
       PyErr_Format (PyExc_ValueError, "Corrupt input at byte %d", -output_size);
       Py_CLEAR (py_dest);
     }
+  else if ((size_t)output_size != dest_size)
+    {
+      // IMHO, it's better to fail explicitly than to allow fishy data to pass through.
+      PyErr_Format (PyExc_ValueError, "Decompressor writes %d bytes, but %zu are claimed", output_size, dest_size);
+      Py_CLEAR (py_dest);
+    }
 
   return py_dest;
 }
diff --git a/lz4/frame/_frame.c b/lz4/frame/_frame.c
@@ -62,6 +62,8 @@ typedef unsigned int uint32_t;
 #define inline
 #endif
 
+static const char * capsule_name = "_frame.LZ4F_cctx";
+static void destruct_compression_context (PyObject * py_context);
 struct compression_context
 {
   LZ4F_compressionContext_t compression_context;
@@ -114,57 +116,26 @@ create_compression_context (PyObject * Py_UNUSED (self),
       return NULL;
     }
 
-  return PyCapsule_New (context, NULL, NULL);
+  return PyCapsule_New (context, capsule_name, destruct_compression_context);
 }
 
-/****************************
- * free_compression_context *
- ****************************/
-PyDoc_STRVAR(free_compression_context__doc,
-             "free_compression_context(context)\n\n"                                \
-             "Releases the resources held by a compression context previously\n" \
-             "created with create_compression_context.\n\n"             \
-             "Args:\n"                                                  \
-             "    context (cCtx): Compression context.\n"
-             );
-
-static PyObject *
-free_compression_context (PyObject * Py_UNUSED (self), PyObject * args,
-                          PyObject * keywds)
+static void
+destruct_compression_context (PyObject * py_context)
 {
-  PyObject *py_context = NULL;
-  static char *kwlist[] = { "context", NULL };
-  struct compression_context *context;
-  LZ4F_errorCode_t result;
-
-  if (!PyArg_ParseTupleAndKeywords (args, keywds, "O", kwlist, &py_context))
-    {
-      return NULL;
-    }
-
-  context =
-    (struct compression_context *) PyCapsule_GetPointer (py_context, NULL);
-  if (!context)
-    {
-      PyErr_Format (PyExc_ValueError, "No compression context supplied");
-      return NULL;
-    }
+  struct compression_context *context =
+#ifndef PyCapsule_Type
+    PyCapsule_GetPointer (py_context, capsule_name);
+    // That's always true as there is no PyCapsule_SetPointer calls.
+#else
+    py_context; // compatibility with 2.6 via capsulethunk
+#endif
 
   Py_BEGIN_ALLOW_THREADS
-  result =
-    LZ4F_freeCompressionContext (context->compression_context);
+  LZ4F_freeCompressionContext (context->compression_context);
+  // That's always LZ4F_OK_NoError as free() is `void free()` and it's just a wrapper.
   Py_END_ALLOW_THREADS
 
-  if (LZ4F_isError (result))
-    {
-      PyErr_Format (PyExc_RuntimeError,
-                    "LZ4F_freeCompressionContext failed with code: %s",
-                    LZ4F_getErrorName (result));
-      return NULL;
-    }
   PyMem_Free (context);
-
-  Py_RETURN_NONE;
 }
 
 /******************
@@ -185,9 +156,10 @@ free_compression_context (PyObject * Py_UNUSED (self), PyObject * args,
   "        - BLOCKMODE_LINKED or 1: linked mode\n\n"                    \
   "        The default is BLOCKMODE_INDEPENDENT.\n"                     \
   "    compression_level (int): Specifies the level of compression used.\n" \
-  "        Values between 0-16 are valid, with 0 (default) being the\n" \
-  "        lowest compression, and 16 the highest. Values above 16 will\n" \
-  "        be treated as 16. Values betwee 3-6 are recommended.\n"      \
+  "        Values between 0-16 are valid, with 0 (default) being the\n"     \
+  "        lowest compression (0-2 are the same value), and 16 the highest.\n" \
+  "        Values above 16 will be treated as 16.\n"             \
+  "        Values between 4-9 are recommended.\n"      \
   "        The following module constants are provided as a convenience:\n\n" \
   "        - COMPRESSIONLEVEL_MIN: Minimum compression (0, the default)\n" \
   "        - COMPRESSIONLEVEL_MINHC: Minimum high-compression mode (3)\n" \
@@ -381,7 +353,7 @@ compress_begin (PyObject * Py_UNUSED (self), PyObject * args,
   preferences.frameInfo.contentSize = source_size;
 
   context =
-    (struct compression_context *) PyCapsule_GetPointer (py_context, NULL);
+    (struct compression_context *) PyCapsule_GetPointer (py_context, capsule_name);
 
   if (!context || !context->compression_context)
     {
@@ -448,7 +420,7 @@ compress_update (PyObject * Py_UNUSED (self), PyObject * args,
     }
 
   context =
-    (struct compression_context *) PyCapsule_GetPointer (py_context, NULL);
+    (struct compression_context *) PyCapsule_GetPointer (py_context, capsule_name);
   if (!context || !context->compression_context)
     {
       PyErr_Format (PyExc_ValueError, "No compression context supplied");
@@ -542,7 +514,7 @@ compress_end (PyObject * Py_UNUSED (self), PyObject * args, PyObject * keywds)
     }
 
   context =
-    (struct compression_context *) PyCapsule_GetPointer (py_context, NULL);
+    (struct compression_context *) PyCapsule_GetPointer (py_context, capsule_name);
   if (!context || !context->compression_context)
     {
       PyErr_SetString (PyExc_ValueError, "No compression context supplied");
@@ -805,6 +777,7 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * keywds)
         }
 
       destination_written += destination_write;
+      source_cursor += source_read;
 
       if (result == 0)
         {
@@ -813,11 +786,10 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * keywds)
 
       if (destination_written == destination_size)
         {
-          /* Destination_buffer is full, so need to expand it. We'll expand
-             it by the approximate size needed from the return value - see
-             LZ4 docs. */
-          destination_size += result;
-          if (!PyMem_Realloc(destination_buffer, destination_size))
+          /* Destination_buffer is full, so need to expand it. */
+          destination_size *= 2;
+          char * nextgen = PyMem_Realloc(destination_buffer, destination_size);
+          if (!nextgen)
             {
               LZ4F_freeDecompressionContext (context);
               Py_BLOCK_THREADS
@@ -826,14 +798,14 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * keywds)
               PyMem_Free (destination_buffer);
               return NULL;
             }
+          destination_buffer = nextgen;
         }
       /* Data still remaining to be decompressed, so increment the source and
          destination cursor locations, and reset source_read and
          destination_write ready for the next iteration. Important to
          re-initialize destination_cursor here (as opposed to simply
          incrementing it) so we're pointing to the realloc'd memory location. */
       destination_cursor = destination_buffer + destination_written;
-      source_cursor += source_read;
       destination_write = destination_size - destination_written;
       source_read = source_end - source_cursor;
     }
@@ -850,6 +822,13 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * keywds)
                     LZ4F_getErrorName (result));
       return NULL;
     }
+  else if (source_cursor != source_end)
+    {
+      PyMem_Free (destination_buffer);
+      PyErr_Format (PyExc_ValueError,
+                    "Extra data: %zd trailing bytes", source_end - source_cursor);
+      return NULL;
+    }
 
   py_dest = PyBytes_FromStringAndSize (destination_buffer, destination_written);
 
@@ -869,10 +848,6 @@ static PyMethodDef module_methods[] =
     "create_compression_context", (PyCFunction) create_compression_context,
     METH_VARARGS | METH_KEYWORDS, create_compression_context__doc
   },
-  {
-    "free_compression_context", (PyCFunction) free_compression_context,
-    METH_VARARGS | METH_KEYWORDS, free_compression_context__doc
-  },
   {
     "compress", (PyCFunction) compress,
     METH_VARARGS | METH_KEYWORDS, compress__doc
diff --git a/setup.py b/setup.py
@@ -11,7 +11,7 @@ def version_scheme(version):
     version = guess_next_dev_version(version)
     return version.lstrip("v")
 
-LZ4_VERSION = "r131"
+LZ4_VERSION = "1.7.4.2"
 
 def library_is_installed(libname):
     # Check to see if we have a library called'libname' installed on the
diff --git a/tests/test_block.py b/tests/test_block.py
@@ -170,6 +170,39 @@ def roundtriphc(x):
         assert data == out
         pool.close()
 
+    def test_block_format(self):
+        data = lz4.compress(b'A' * 64)
+        self.assertEqual(data[:4], b'\x40\0\0\0')
+        self.assertEqual(lz4.decompress(data[4:], uncompressed_size=64), b'A' * 64)
+
+class TestLZ4BlockModern(unittest.TestCase):
+    def test_decompress_ui32_overflow(self):
+        data = lz4.compress(b'A' * 64)
+        with self.assertRaisesRegexp(OverflowError, r'^signed integer is greater than maximum$'):
+            lz4.decompress(data[4:], uncompressed_size=((1<<32) + 64))
+
+    def test_decompress_without_leak(self):
+        # Verify that hand-crafted packet does not leak uninitialized(?) memory.
+        data = lz4.compress(b'A' * 64)
+        with self.assertRaisesRegexp(ValueError, r'^Decompressor writes 64 bytes, but 79 are claimed$'):
+            lz4.decompress(b'\x4f' + data[1:])
+        with self.assertRaisesRegexp(ValueError, r'^Decompressor writes 64 bytes, but 79 are claimed$'):
+            lz4.decompress(data[4:], uncompressed_size=79)
+
+    def test_decompress_with_trailer(self):
+        data = b'A' * 64
+        comp = lz4.compress(data)
+        with self.assertRaisesRegexp(ValueError, r'^Corrupt input at byte'):
+            self.assertEqual(data, lz4.block.decompress(comp + b'A'))
+        with self.assertRaisesRegexp(ValueError, r'^Corrupt input at byte'):
+            self.assertEqual(data, lz4.block.decompress(comp + comp))
+        with self.assertRaisesRegexp(ValueError, r'^Corrupt input at byte'):
+            self.assertEqual(data, lz4.block.decompress(comp + comp[4:]))
+
+if sys.version_info < (2, 7):
+    # Poor-man unittest.TestCase.skip for Python 2.6
+    del TestLZ4BlockModern
+
 if __name__ == '__main__':
     unittest.main()
 
diff --git a/tests/test_frame.py b/tests/test_frame.py
@@ -1,13 +1,13 @@
 import lz4.frame as lz4frame
 import unittest
 import os
+import sys
 from multiprocessing.pool import ThreadPool
 
 class TestLZ4Frame(unittest.TestCase):
     def test_create_and_free_compression_context(self):
         context = lz4frame.create_compression_context()
         self.assertNotEqual(context, None)
-        lz4frame.free_compression_context(context)
 
     def test_compress(self):
         input_data = b"2099023098234882923049823094823094898239230982349081231290381209380981203981209381238901283098908123109238098123"
@@ -25,7 +25,28 @@ def test_compress_begin_update_end(self):
         compressed += lz4frame.compress_update(context, input_data[:chunk_size])
         compressed += lz4frame.compress_update(context, input_data[chunk_size:])
         compressed += lz4frame.compress_end(context)
-        lz4frame.free_compression_context(context)
+        decompressed = lz4frame.decompress(compressed)
+        self.assertEqual(input_data, decompressed)
+
+    def test_compress_huge_with_size(self):
+        context = lz4frame.create_compression_context()
+        input_data = b"2099023098234882923049823094823094898239230982349081231290381209380981203981209381238901283098908123109238098123" * 4096
+        chunk_size = int((len(input_data)/2)+1)
+        compressed = lz4frame.compress_begin(context, source_size=len(input_data))
+        compressed += lz4frame.compress_update(context, input_data[:chunk_size])
+        compressed += lz4frame.compress_update(context, input_data[chunk_size:])
+        compressed += lz4frame.compress_end(context)
+        decompressed = lz4frame.decompress(compressed)
+        self.assertEqual(input_data, decompressed)
+
+    def test_compress_huge_without_size(self):
+        context = lz4frame.create_compression_context()
+        input_data = b"2099023098234882923049823094823094898239230982349081231290381209380981203981209381238901283098908123109238098123" * 4096
+        chunk_size = int((len(input_data)/2)+1)
+        compressed = lz4frame.compress_begin(context)
+        compressed += lz4frame.compress_update(context, input_data[:chunk_size])
+        compressed += lz4frame.compress_update(context, input_data[chunk_size:])
+        compressed += lz4frame.compress_end(context)
         decompressed = lz4frame.decompress(compressed)
         self.assertEqual(input_data, decompressed)
 
@@ -86,7 +107,6 @@ def test_compress_begin_update_end_no_auto_flush(self):
         compressed += lz4frame.compress_update(context, input_data[:chunk_size])
         compressed += lz4frame.compress_update(context, input_data[chunk_size:])
         compressed += lz4frame.compress_end(context)
-        lz4frame.free_compression_context(context)
         decompressed = lz4frame.decompress(compressed)
         self.assertEqual(input_data, decompressed)
 
@@ -105,7 +125,6 @@ def test_compress_begin_update_end_no_auto_flush_2(self):
             end = start + chunk_size
 
         compressed += lz4frame.compress_end(context)
-        lz4frame.free_compression_context(context)
         decompressed = lz4frame.decompress(compressed)
         self.assertEqual(input_data, decompressed)
 
@@ -130,7 +149,6 @@ def test_compress_begin_update_end_not_defaults(self):
             end = start + chunk_size
 
         compressed += lz4frame.compress_end(context)
-        lz4frame.free_compression_context(context)
         decompressed = lz4frame.decompress(compressed)
         self.assertEqual(input_data, decompressed)
 
@@ -155,7 +173,6 @@ def test_compress_begin_update_end_no_auto_flush_not_defaults(self):
             end = start + chunk_size
 
         compressed += lz4frame.compress_end(context)
-        lz4frame.free_compression_context(context)
         decompressed = lz4frame.decompress(compressed)
         self.assertEqual(input_data, decompressed)
 
@@ -216,7 +233,6 @@ def roundtrip(x):
                 end = start + chunk_size
 
             compressed += lz4frame.compress_end(context)
-            lz4frame.free_compression_context(context)
             decompressed = lz4frame.decompress(compressed)
             return decompressed
 
@@ -225,5 +241,19 @@ def roundtrip(x):
         pool.close()
         self.assertEqual(data, out)
 
+class TestLZ4FrameModern(unittest.TestCase):
+    def test_decompress_trailer(self):
+        input_data = b"2099023098234882923049823094823094898239230982349081231290381209380981203981209381238901283098908123109238098123"
+        compressed = lz4frame.compress(input_data)
+        with self.assertRaisesRegexp(ValueError, r'^Extra data: 64 trailing bytes'):
+            lz4frame.decompress(compressed + b'A'*64)
+        # This API does not support frame concatenation!
+        with self.assertRaisesRegexp(ValueError, r'^Extra data: \d+ trailing bytes'):
+            lz4frame.decompress(compressed + compressed)
+
+if sys.version_info < (2, 7):
+    # Poor-man unittest.TestCase.skip for Python 2.6
+    del TestLZ4FrameModern
+
 if __name__ == '__main__':
     unittest.main()

Original file line number	Diff line number	Diff line change
`@@ -283,6 +283,12 @@ decompress (PyObject * Py_UNUSED (self), PyObject * args, PyObject * kwargs)`
`283`	`283`	`PyErr_Format (PyExc_ValueError, "Corrupt input at byte %d", -output_size);`
`284`	`284`	`Py_CLEAR (py_dest);`
`285`	`285`	`}`
	`286`	`+ else if ((size_t)output_size != dest_size)`
	`287`	`+ {`
	`288`	`+ // IMHO, it's better to fail explicitly than to allow fishy data to pass through.`
	`289`	`+ PyErr_Format (PyExc_ValueError, "Decompressor writes %d bytes, but %zu are claimed", output_size, dest_size);`
	`290`	`+ Py_CLEAR (py_dest);`
	`291`	`+ }`
`286`	`292`
`287`	`293`	`return py_dest;`
`288`	`294`	`}`