File u_tigervnc-1.3.0-fix-use-after-free.patch of Package tigervnc

Overview Repositories Revisions Requests Users Attributes Meta

File u_tigervnc-1.3.0-fix-use-after-free.patch of Package tigervnc (Revision 0645d76ce33952bef6431d09f7877459)

Currently displaying revision 0645d76ce33952bef6431d09f7877459 , Show latest

Author: Michal Srb <msrb@suse.com>
Subject: Fix use after free in ZRLEEncoder.
Patch-Mainline: To be upstreamed
References: bnc#840433

There is use after free crash when client using zrle disconnects:
ZRLEEncoder contains zos variable (rdr::ZlibOutStream) and mos variable (pointer to rdr::MemOutStream).
mos is always allocated in constructor (it could be a copy of static sharedMos pointer if sharedMos != 0, but it is always 0). 
When ZRLEEncoder::writeRect is called, any of zrleEncode* functions sets mos as an underlying stream of zos.
When ZRLEEncoder is destructed, mos is deleted (sharedMos is always 0), then zos is implicitly destructed, but zos accesses it's underlying stream in it's destructor!

We need to destruct mos first and zos second when ZRLEEncoder is destructed.
As sharedMos is never used, we can remove that, simplify ZRLEEncoder and turn zos into a member variable same as mos. They will be both implicitly destructed in reverse order of declaration.

diff -ur tigervnc-1.3.0-orig/common/rfb/ZRLEEncoder.cxx tigervnc-1.3.0/common/rfb/ZRLEEncoder.cxx
--- tigervnc-1.3.0-orig/common/rfb/ZRLEEncoder.cxx	2013-09-17 00:18:28.557911306 +0300
+++ tigervnc-1.3.0/common/rfb/ZRLEEncoder.cxx	2013-09-17 00:19:57.487915741 +0300
@@ -26,7 +26,6 @@
 
 using namespace rfb;
 
-rdr::MemOutStream* ZRLEEncoder::sharedMos = 0;
 int ZRLEEncoder::maxLen = 4097 * 1024; // enough for width 16384 32-bit pixels
 
 IntParameter zlibLevel("ZlibLevel","Zlib compression level",-1);
@@ -55,33 +54,27 @@
 }
 
 ZRLEEncoder::ZRLEEncoder(SMsgWriter* writer_)
-  : writer(writer_), zos(0,0,zlibLevel)
+  : writer(writer_), zos(0,0,zlibLevel), mos(129*1024)
 {
-  if (sharedMos)
-    mos = sharedMos;
-  else
-    mos = new rdr::MemOutStream(129*1024);
 }
 
 ZRLEEncoder::~ZRLEEncoder()
 {
-  if (!sharedMos)
-    delete mos;
 }
 
 bool ZRLEEncoder::writeRect(const Rect& r, TransImageGetter* ig, Rect* actual)
 {
   rdr::U8* imageBuf = writer->getImageBuf(64 * 64 * 4 + 4);
-  mos->clear();
+  mos.clear();
   bool wroteAll = true;
   *actual = r;
 
   switch (writer->bpp()) {
   case 8:
-    wroteAll = zrleEncode8(r, mos, &zos, imageBuf, maxLen, actual, ig);
+    wroteAll = zrleEncode8(r, &mos, &zos, imageBuf, maxLen, actual, ig);
     break;
   case 16:
-    wroteAll = zrleEncode16(r, mos, &zos, imageBuf, maxLen, actual, ig);
+    wroteAll = zrleEncode16(r, &mos, &zos, imageBuf, maxLen, actual, ig);
     break;
   case 32:
     {
@@ -94,16 +87,16 @@
       if ((fitsInLS3Bytes && pf.isLittleEndian()) ||
           (fitsInMS3Bytes && pf.isBigEndian()))
       {
-        wroteAll = zrleEncode24A(r, mos, &zos, imageBuf, maxLen, actual, ig);
+        wroteAll = zrleEncode24A(r, &mos, &zos, imageBuf, maxLen, actual, ig);
       }
       else if ((fitsInLS3Bytes && pf.isBigEndian()) ||
                (fitsInMS3Bytes && pf.isLittleEndian()))
       {
-        wroteAll = zrleEncode24B(r, mos, &zos, imageBuf, maxLen, actual, ig);
+        wroteAll = zrleEncode24B(r, &mos, &zos, imageBuf, maxLen, actual, ig);
       }
       else
       {
-        wroteAll = zrleEncode32(r, mos, &zos, imageBuf, maxLen, actual, ig);
+        wroteAll = zrleEncode32(r, &mos, &zos, imageBuf, maxLen, actual, ig);
       }
       break;
     }
@@ -111,8 +104,8 @@
 
   writer->startRect(*actual, encodingZRLE);
   rdr::OutStream* os = writer->getOutStream();
-  os->writeU32(mos->length());
-  os->writeBytes(mos->data(), mos->length());
+  os->writeU32(mos.length());
+  os->writeBytes(mos.data(), mos.length());
   writer->endRect();
   return wroteAll;
 }
diff -ur tigervnc-1.3.0-orig/common/rfb/ZRLEEncoder.h tigervnc-1.3.0/common/rfb/ZRLEEncoder.h
--- tigervnc-1.3.0-orig/common/rfb/ZRLEEncoder.h	2013-09-17 00:18:28.558911306 +0300
+++ tigervnc-1.3.0/common/rfb/ZRLEEncoder.h	2013-09-17 00:20:34.372917581 +0300
@@ -38,16 +38,11 @@
     // width, in this example about 128 bytes).
     static void setMaxLen(int m) { maxLen = m; }
 
-    // setSharedMos() sets a MemOutStream to be shared amongst all
-    // ZRLEEncoders.  Should be called before any ZRLEEncoders are created.
-    static void setSharedMos(rdr::MemOutStream* mos_) { sharedMos = mos_; }
-
   private:
     ZRLEEncoder(SMsgWriter* writer);
     SMsgWriter* writer;
+    rdr::MemOutStream mos;
     rdr::ZlibOutStream zos;
-    rdr::MemOutStream* mos;
-    static rdr::MemOutStream* sharedMos;
     static int maxLen;
   };
 }

Places

File u_tigervnc-1.3.0-fix-use-after-free.patch of Package tigervnc (Revision 0645d76ce33952bef6431d09f7877459)

Places