Bounds-check the MP parser and fix an invalid free on valid MPO files

lib/kodi-libmpo-note.txt

@@ -1,2 +1,7 @@
 libmpo source from https://github.com/Lectem/libmpo
-Sync to 63ada10 (14 Aug 2017)
+Sync to 63ada10 (14 Aug 2017)
+
+Locally modified, not present upstream (upstream's last code commit is 2017):
+  src/mpo.c - mpf_getbyte(), mpf_seek() and mpf_dc_rewindc() bounds-check the
+  cursor. The only check was an assert() in mpf_getbyte(), which NDEBUG removes
+  from release builds, leaving every read in the MP parser unchecked.

lib/libmpo/include/libmpo/mpo.h

@@ -46,6 +46,9 @@ typedef struct {MPFLong numerator; MPFLong denominator;} MPFRational;
 typedef unsigned char	MPFUndefined;     /* 1 byte  */
 typedef struct {MPFSLong numerator; MPFSLong denominator;} MPFSRational;
 
+/* Kodi downstream: a sanity ceiling on the file-supplied image count. */
+#define MPO_MAX_IMAGES 64
+
 typedef struct
 {
     MPFByte * buffer;

lib/libmpo/src/dmpo.c

@@ -122,8 +122,34 @@ bool mpo_read_header(mpo_decompress_struct *mpoinfo) {
 
         res = jpeg_read_header((j_decompress_ptr) &mpoinfo->cinfo, TRUE) != 0;
         int nbImages = mpoinfo->APP02->numberOfImages;
-        if(nbImages > 1)
-            mpoinfo->APP02 = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02));
+        /* Kodi downstream: numberOfImages comes from the file, and
+           mpo_destroy_decompress() walks every entry calling free() on its
+           MPentry. realloc() does not zero what it adds, and only entry 0 has
+           been parsed at this point, so the rest must be cleared or the
+           teardown frees uninitialised pointers. */
+        if(nbImages > 1 && nbImages <= MPO_MAX_IMAGES)
+        {
+            MPExt_Data *grown = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02));
+            if(grown)
+            {
+                memset(grown + 1, 0, (nbImages - 1) * (sizeof *grown));
+                mpoinfo->APP02 = grown;
+            }
+            else
+            {
+                /* Only one entry is allocated, and mpo_destroy_decompress()
+                   walks numberOfImages of them, so it has to agree. */
+                mpoinfo->APP02->numberOfImages = 1;
+                res = 0;
+            }
+        }
+        else if(nbImages > MPO_MAX_IMAGES)
+        {
+            mpoinfo->APP02->numberOfImages = 1;
+            res = 0;
+        }
+        else if(nbImages < 1)
+            mpoinfo->APP02->numberOfImages = 1;
 
     }
     return res;

lib/libmpo/src/mpo.c

@@ -20,21 +20,29 @@ long mpf_tell(MPFbuffer_ptr b)
 
 void mpf_seek(MPFbuffer_ptr b,long offset, int from)
 {
-    if(from==SEEK_CUR)b->_cur+=offset;
-    else if(from==SEEK_SET)b->_cur=offset;
-    else if(from==SEEK_END)b->_cur=b->_size-1+offset;
+    /* Kodi downstream: offsets come from the file, so clamp rather than trust. */
+    long target;
+    if(from==SEEK_CUR)target=b->_cur+offset;
+    else if(from==SEEK_SET)target=offset;
+    else if(from==SEEK_END)target=b->_size-1+offset;
+    else return;
+    if(target<0)target=0;
+    if(target>b->_size)target=b->_size;
+    b->_cur=target;
 }
 
 unsigned int mpf_getbyte (MPFbuffer_ptr b)
 /* Read next byte */
 {
-    assert(b->_cur < b->_size);
+    /* Kodi downstream: this assert was the only bounds check on every read in
+       the MP parser, and NDEBUG removes it from the builds we ship. */
+    if(b->_cur < 0 || b->_cur >= b->_size)return 0;
     return b->buffer[b->_cur++];
 }
 
 void mpf_dc_rewindc(MPFbuffer_ptr b)
 {
-    b->_cur--;
+    if(b->_cur>0)b->_cur--;
 }
 
 uint16_t mpf_getint16 (MPFbuffer_ptr b, int swapEndian)

src/MPOPicture.cpp

@@ -10,9 +10,30 @@
 
 #include "../lib/TinyEXIF/TinyEXIF.h"
 
+#include <csetjmp>
 #include <iostream>
 #include <kodi/Filesystem.h>
 
+namespace
+{
+
+// libjpeg's default error_exit calls exit(), and libmpo installs it via
+// jpeg_std_error() without overriding it, so a malformed file terminates Kodi
+// rather than failing the decode. mpo_decompress_error_exit() can only replace
+// the function pointer, not attach state to it, so the jump target is
+// thread-local; each decoder instance is driven from one thread at a time.
+thread_local std::jmp_buf s_jpegEscape;
+
+void MpoFatalError(j_common_ptr cinfo)
+{
+  char message[JMSG_LENGTH_MAX] = {};
+  (*cinfo->err->format_message)(cinfo, message);
+  kodi::Log(ADDON_LOG_ERROR, "libjpeg: %s", message);
+  std::longjmp(s_jpegEscape, 1);
+}
+
+} // namespace
+
 MPOPicture::MPOPicture(const kodi::addon::IInstanceInfo& instance)
   : CInstanceImageDecoder(instance)
 {
@@ -36,8 +57,15 @@ bool MPOPicture::SupportsFile(const std::string& file)
 
   mpo_decompress_struct mpoinfo;
   mpo_create_decompress(&mpoinfo);
+  mpo_decompress_error_exit(&mpoinfo, MpoFatalError);
+  if (setjmp(s_jpegEscape))
+  {
+    mpo_destroy_decompress(&mpoinfo);
+    return false;
+  }
+
   mpo_mem_src(&mpoinfo, buffer.data(), buffer.size());
-  bool ret = mpo_read_header(&mpoinfo);
+  const bool ret = mpo_read_header(&mpoinfo);
   mpo_destroy_decompress(&mpoinfo);
   return ret;
 }
@@ -139,6 +167,13 @@ bool MPOPicture::LoadImageFromMemory(const std::string& mimetype,
   m_data.resize(bufSize);
   std::copy(buffer, buffer + bufSize, m_data.begin());
   mpo_create_decompress(&m_mpoinfo);
+  mpo_decompress_error_exit(&m_mpoinfo, MpoFatalError);
+  if (setjmp(s_jpegEscape))
+  {
+    mpo_destroy_decompress(&m_mpoinfo);
+    return false;
+  }
+
   mpo_mem_src(&m_mpoinfo, m_data.data(), m_data.size());
   if (!mpo_read_header(&m_mpoinfo))
   {
@@ -169,6 +204,9 @@ bool MPOPicture::Decode(uint8_t* pixels,
     return false;
   }
 
+  if (setjmp(s_jpegEscape))
+    return false;
+
   size_t image = 0;
   while (image < m_images)
   {