Attachment 558340 Details for Bug 840433

[patch] Fix use after free.

u_tigervnc-1.3.0-fix-use-after-free.patch (text/plain), 4.29 KB, created by Michal Srb on 2013-09-16 22:15:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Michal Srb

Created: 2013-09-16 22:15:04 UTC

Size: 4.29 KB

patch

obsolete

>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;
>   };
> }

Actions: View | Diff

Attachments on bug 840433: 558340