File 0002-Encapsulate-PixelBuffer-internal-details.p... of Package tigervnc.20176

Overview Repositories Revisions Requests Users Attributes Meta

File 0002-Encapsulate-PixelBuffer-internal-details.patch of Package tigervnc.20176

From 53f913a76196c7357d4858bfbf2c33caa9181bae Mon Sep 17 00:00:00 2001
From: Pierre Ossman <ossman@cendio.se>
Date: Tue, 10 Sep 2019 15:18:30 +0200
Subject: [PATCH] Encapsulate PixelBuffer internal details

Don't allow subclasses to just override dimensions or buffer details
directly and instead force them to go via methods. This allows us
to do sanity checks on the new values and catch bugs and attacks.
---
 common/rfb/Cursor.cxx                 |   3 +-
 common/rfb/EncodeManager.cxx          |   5 +-
 common/rfb/PixelBuffer.cxx            | 103 +++++++++++++++++++++-------------
 common/rfb/PixelBuffer.h              |  17 ++++--
 unix/x0vncserver/XPixelBuffer.cxx     |   9 +--
 unix/xserver/hw/vnc/XserverDesktop.cc |  24 ++++----
 unix/xserver/hw/vnc/XserverDesktop.h  |   2 +-
 vncviewer/PlatformPixelBuffer.cxx     |   9 ++-
 win/rfb_win32/DIBSectionBuffer.cxx    |  41 ++++++--------
 9 files changed, 111 insertions(+), 102 deletions(-)

Index: tigervnc-1.8.0/common/rfb/Cursor.cxx
===================================================================
--- tigervnc-1.8.0.orig/common/rfb/Cursor.cxx
+++ tigervnc-1.8.0/common/rfb/Cursor.cxx
@@ -271,8 +271,7 @@ void RenderedCursor::update(PixelBuffer*
   assert(cursor);
 
   format = framebuffer->getPF();
-  width_ = framebuffer->width();
-  height_ = framebuffer->height();
+  setSize(framebuffer->width(), framebuffer->height());
 
   rawOffset = pos.subtract(cursor->hotspot());
   clippedRect = Rect(0, 0, cursor->width(), cursor->height())
Index: tigervnc-1.8.0/common/rfb/EncodeManager.cxx
===================================================================
--- tigervnc-1.8.0.orig/common/rfb/EncodeManager.cxx
+++ tigervnc-1.8.0/common/rfb/EncodeManager.cxx
@@ -891,11 +891,8 @@ void EncodeManager::OffsetPixelBuffer::u
                                               int stride_)
 {
   format = pf;
-  width_ = width;
-  height_ = height;
   // Forced cast. We never write anything though, so it should be safe.
-  data = (rdr::U8*)data_;
-  stride = stride_;
+  setBuffer(width, height, (rdr::U8*)data_, stride_);
 }
 
 // Preprocessor generated, optimised methods
Index: tigervnc-1.8.0/common/rfb/PixelBuffer.cxx
===================================================================
--- tigervnc-1.8.0.orig/common/rfb/PixelBuffer.cxx
+++ tigervnc-1.8.0/common/rfb/PixelBuffer.cxx
@@ -35,8 +35,14 @@ static LogWriter vlog("PixelBuffer");
 // -=- Generic pixel buffer class
 
 PixelBuffer::PixelBuffer(const PixelFormat& pf, int w, int h)
-  : format(pf), width_(w), height_(h) {}
-PixelBuffer::PixelBuffer() : width_(0), height_(0) {}
+  : format(pf), width_(0), height_(0)
+{
+  setSize(w, h);
+}
+
+PixelBuffer::PixelBuffer() : width_(0), height_(0)
+{
+}
 
 PixelBuffer::~PixelBuffer() {}
 
@@ -53,7 +59,7 @@ PixelBuffer::getImage(void* imageBuf, co
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          r.width(), r.height(),
-                         r.tl.x, r.tl.y, width_, height_);
+                         r.tl.x, r.tl.y, width(), height());
 
   data = getBuffer(r, &inStride);
 
@@ -89,7 +95,7 @@ void PixelBuffer::getImage(const PixelFo
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          r.width(), r.height(),
-                         r.tl.x, r.tl.y, width_, height_);
+                         r.tl.x, r.tl.y, width(), height());
 
   if (stride == 0)
     stride = r.width();
@@ -100,6 +106,12 @@ void PixelBuffer::getImage(const PixelFo
                       stride, srcStride);
 }
 
+void PixelBuffer::setSize(int width, int height)
+{
+  width_ = width;
+  height_ = height;
+}
+
 // -=- Modifiable generic pixel buffer class
 
 ModifiablePixelBuffer::ModifiablePixelBuffer(const PixelFormat& pf,
@@ -124,7 +136,7 @@ void ModifiablePixelBuffer::fillRect(con
 
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
-                         r.width(), r.height(), r.tl.x, r.tl.y, width_, height_);
+                         r.width(), r.height(), r.tl.x, r.tl.y, width(), height());
 
   w = r.width();
   h = r.height();
@@ -175,7 +187,7 @@ void ModifiablePixelBuffer::imageRect(co
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          r.width(), r.height(),
-                         r.tl.x, r.tl.y, width_, height_);
+                         r.tl.x, r.tl.y, width(), height());
 
   bytesPerPixel = getPF().bpp/8;
 
@@ -213,13 +225,13 @@ void ModifiablePixelBuffer::copyRect(con
   if (!drect.enclosed_by(getRect()))
     throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          drect.width(), drect.height(),
-                         drect.tl.x, drect.tl.y, width_, height_);
+                         drect.tl.x, drect.tl.y, width(), height());
 
   srect = drect.translate(move_by_delta.negate());
   if (!srect.enclosed_by(getRect()))
     throw rfb::Exception("Source rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          srect.width(), srect.height(),
-                         srect.tl.x, srect.tl.y, width_, height_);
+                         srect.tl.x, srect.tl.y, width(), height());
 
   srcData = getBuffer(srect, &srcStride);
   dstData = getBufferRW(drect, &dstStride);
@@ -272,7 +284,7 @@ void ModifiablePixelBuffer::imageRect(co
   if (!dest.enclosed_by(getRect()))
     throw rfb::Exception("Destination rect %dx%d at %d,%d exceeds framebuffer %dx%d",
                          dest.width(), dest.height(),
-                         dest.tl.x, dest.tl.y, width_, height_);
+                         dest.tl.x, dest.tl.y, width(), height());
 
   if (stride == 0)
     stride = dest.width();
@@ -301,7 +313,7 @@ rdr::U8* FullFramePixelBuffer::getBuffer
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Pixel buffer request %dx%d at %d,%d exceeds framebuffer %dx%d",
                          r.width(), r.height(),
-                         r.tl.x, r.tl.y, width_, height_);
+                         r.tl.x, r.tl.y, width(), height());
 
   *stride_ = stride;
   return &data[(r.tl.x + (r.tl.y * stride)) * format.bpp/8];
@@ -316,55 +328,67 @@ const rdr::U8* FullFramePixelBuffer::get
   if (!r.enclosed_by(getRect()))
     throw rfb::Exception("Pixel buffer request %dx%d at %d,%d exceeds framebuffer %dx%d",
                          r.width(), r.height(),
-                         r.tl.x, r.tl.y, width_, height_);
+                         r.tl.x, r.tl.y, width(), height());
 
   *stride_ = stride;
   return &data[(r.tl.x + (r.tl.y * stride)) * format.bpp/8];
 }
 
+void FullFramePixelBuffer::setBuffer(int width, int height,
+                                     rdr::U8* data_, int stride_)
+{
+  ModifiablePixelBuffer::setSize(width, height);
+  stride = stride_;
+  data = data_;
+}
+
+void FullFramePixelBuffer::setSize(int w, int h)
+{
+  // setBuffer() should be used
+  throw rfb::Exception("Invalid call to FullFramePixelBuffer::setSize()");
+}
+
 // -=- Managed pixel buffer class
 // Automatically allocates enough space for the specified format & area
 
 ManagedPixelBuffer::ManagedPixelBuffer()
-  : datasize(0)
+  : data_(NULL), datasize(0)
 {
-  checkDataSize();
-};
+}
 
 ManagedPixelBuffer::ManagedPixelBuffer(const PixelFormat& pf, int w, int h)
-  : FullFramePixelBuffer(pf, w, h, NULL, w), datasize(0)
+  : FullFramePixelBuffer(pf, 0, 0, NULL, 0), data_(NULL), datasize(0)
 {
-  checkDataSize();
-};
-
-ManagedPixelBuffer::~ManagedPixelBuffer() {
-  if (data) delete [] data;
-};
-
+  setSize(w, h);
+}
 
-void
-ManagedPixelBuffer::setPF(const PixelFormat &pf) {
-  format = pf; checkDataSize();
-};
-void
-ManagedPixelBuffer::setSize(int w, int h) {
-  width_ = w; height_ = h; stride = w; checkDataSize();
-};
+ManagedPixelBuffer::~ManagedPixelBuffer()
+{
+  if (data_)
+    delete [] data_;
+}
 
+void ManagedPixelBuffer::setPF(const PixelFormat &pf)
+{
+  format = pf;
+  setSize(width(), height());
+}
+ 
+void ManagedPixelBuffer::setSize(int w, int h)
+{
+  unsigned long new_datasize = w * h * (format.bpp/8);
 
-inline void
-ManagedPixelBuffer::checkDataSize() {
-  unsigned long new_datasize = width_ * height_ * (format.bpp/8);
+  new_datasize = w * h * (format.bpp/8);
   if (datasize < new_datasize) {
-    if (data) {
-      delete [] data;
-      datasize = 0; data = 0;
+    if (data_) {
+      delete [] data_;
+      data_ = NULL;
+      datasize = 0;
     }
     if (new_datasize) {
-      data = new U8[new_datasize];
-      if (!data)
-        throw Exception("rfb::ManagedPixelBuffer unable to allocate buffer");
+      data_ = new U8[new_datasize];
       datasize = new_datasize;
     }
   }
+  setBuffer(w, h, data_, w);
 };
Index: tigervnc-1.8.0/common/rfb/PixelBuffer.h
===================================================================
--- tigervnc-1.8.0.orig/common/rfb/PixelBuffer.h
+++ tigervnc-1.8.0/common/rfb/PixelBuffer.h
@@ -89,7 +89,12 @@ namespace rfb {
 
   protected:
     PixelBuffer();
+    virtual void setSize(int width, int height);
+
+  protected:
     PixelFormat format;
+
+  private:
     int width_, height_;
   };
 
@@ -153,7 +158,12 @@ namespace rfb {
 
   protected:
     FullFramePixelBuffer();
+    virtual void setBuffer(int width, int height, rdr::U8* data, int stride);
 
+  private:
+    virtual void setSize(int w, int h);
+
+  private:
     rdr::U8* data;
     int stride;
   };
@@ -171,12 +181,9 @@ namespace rfb {
     virtual void setPF(const PixelFormat &pf);
     virtual void setSize(int w, int h);
 
-    // Return the total number of bytes of pixel data in the buffer
-    int dataLen() const { return width_ * height_ * (format.bpp/8); }
-
-  protected:
+  private:
+    rdr::U8* data_; // Mirrors FullFramePixelBuffer::data
     unsigned long datasize;
-    void checkDataSize();
   };
 
 };
Index: tigervnc-1.8.0/unix/x0vncserver/XPixelBuffer.cxx
===================================================================
--- tigervnc-1.8.0.orig/unix/x0vncserver/XPixelBuffer.cxx
+++ tigervnc-1.8.0/unix/x0vncserver/XPixelBuffer.cxx
@@ -50,13 +50,8 @@ XPixelBuffer::XPixelBuffer(Display *dpy,
                        ffs(m_image->xim->blue_mask) - 1);
 
   // Set up the remaining data of the parent class.
-  width_ = rect.width();
-  height_ = rect.height();
-  data = (rdr::U8 *)m_image->xim->data;
-
-  // Calculate the distance in pixels between two subsequent scan
-  // lines of the framebuffer. This may differ from image width.
-  stride = m_image->xim->bytes_per_line * 8 / m_image->xim->bits_per_pixel;
+  setBuffer(rect.width(), rect.height(), (rdr::U8 *)m_image->xim->data,
+            m_image->xim->bytes_per_line * 8 / m_image->xim->bits_per_pixel);
 
   // Get initial screen image from the X display.
   m_image->get(DefaultRootWindow(m_dpy), m_offsetLeft, m_offsetTop);
Index: tigervnc-1.8.0/unix/xserver/hw/vnc/XserverDesktop.cc
===================================================================
--- tigervnc-1.8.0.orig/unix/xserver/hw/vnc/XserverDesktop.cc
+++ tigervnc-1.8.0/unix/xserver/hw/vnc/XserverDesktop.cc
@@ -101,7 +101,7 @@ XserverDesktop::XserverDesktop(int scree
   : screenIndex(screenIndex_),
     server(0), httpServer(0),
     listeners(listeners_), httpListeners(httpListeners_),
-    directFbptr(true),
+    shadowFramebuffer(NULL),
     queryConnectId(0)
 {
   format = pf;
@@ -138,8 +138,8 @@ XserverDesktop::~XserverDesktop()
     delete httpListeners.back();
     httpListeners.pop_back();
   }
-  if (!directFbptr)
-    delete [] data;
+  if (shadowFramebuffer)
+    delete [] shadowFramebuffer;
   delete httpServer;
   delete server;
 }
@@ -158,22 +158,18 @@ void XserverDesktop::setFramebuffer(int
 {
   ScreenSet layout;
 
-  width_ = w;
-  height_ = h;
-
-  if (!directFbptr) {
-    delete [] data;
-    directFbptr = true;
+  if (shadowFramebuffer) {
+    delete [] shadowFramebuffer;
+    shadowFramebuffer = NULL;
   }
 
   if (!fbptr) {
-    fbptr = new rdr::U8[w * h * (format.bpp/8)];
+    shadowFramebuffer = new rdr::U8[w * h * (format.bpp/8)];
+    fbptr = shadowFramebuffer;
     stride_ = w;
-    directFbptr = false;
   }
 
-  data = (rdr::U8*)fbptr;
-  stride = stride_;
+  setBuffer(w, h, (rdr::U8*)fbptr, stride_);
 
   layout = computeScreenLayout();
 
@@ -749,7 +745,7 @@ unsigned int XserverDesktop::setScreenLa
 
 void XserverDesktop::grabRegion(const rfb::Region& region)
 {
-  if (directFbptr)
+  if (shadowFramebuffer == NULL)
     return;
 
   std::vector<rfb::Rect> rects;
Index: tigervnc-1.8.0/unix/xserver/hw/vnc/XserverDesktop.h
===================================================================
--- tigervnc-1.8.0.orig/unix/xserver/hw/vnc/XserverDesktop.h
+++ tigervnc-1.8.0/unix/xserver/hw/vnc/XserverDesktop.h
@@ -121,7 +121,7 @@ private:
   rfb::HTTPServer* httpServer;
   std::list<network::TcpListener*> listeners;
   std::list<network::TcpListener*> httpListeners;
-  bool directFbptr;
+  rdr::U8* shadowFramebuffer;
 
   uint32_t queryConnectId;
   network::Socket* queryConnectSocket;
Index: tigervnc-1.8.0/vncviewer/PlatformPixelBuffer.cxx
===================================================================
--- tigervnc-1.8.0.orig/vncviewer/PlatformPixelBuffer.cxx
+++ tigervnc-1.8.0/vncviewer/PlatformPixelBuffer.cxx
@@ -36,7 +36,7 @@ static rfb::LogWriter vlog("PlatformPixe
 PlatformPixelBuffer::PlatformPixelBuffer(int width, int height) :
   FullFramePixelBuffer(rfb::PixelFormat(32, 24, false, true,
                                        255, 255, 255, 16, 8, 0),
-                       width, height, 0, stride),
+                       0, 0, NULL, 0),
   Surface(width, height)
 #if !defined(WIN32) && !defined(__APPLE__)
   , shminfo(NULL), xim(NULL)
@@ -56,11 +56,10 @@ PlatformPixelBuffer::PlatformPixelBuffer
     vlog.debug("Using standard XImage");
   }
 
-  data = (rdr::U8*)xim->data;
-  stride = xim->bytes_per_line / (getPF().bpp/8);
+  setBuffer(width, height, (rdr::U8*)xim->data,
+            xim->bytes_per_line / (getPF().bpp/8));
 #else
-  FullFramePixelBuffer::data = (rdr::U8*)Surface::data;
-  stride = width;
+  setBuffer(width, height, (rdr::U8*)Surface::data, width);
 #endif
 }
 
Index: tigervnc-1.8.0/win/rfb_win32/DIBSectionBuffer.cxx
===================================================================
--- tigervnc-1.8.0.orig/win/rfb_win32/DIBSectionBuffer.cxx
+++ tigervnc-1.8.0/win/rfb_win32/DIBSectionBuffer.cxx
@@ -52,39 +52,28 @@ void DIBSectionBuffer::setPF(const Pixel
   if (!pf.trueColour)
     throw rfb::Exception("palette format not supported");
   format = pf;
-  recreateBuffer();
+  setSize(width(), height());
 }
 
-void DIBSectionBuffer::setSize(int w, int h) {
-  if (width_ == w && height_ == h) {
-    vlog.debug("size unchanged by setSize()");
-    return;
-  }
-  width_ = w;
-  height_ = h;
-  recreateBuffer();
-}
-
-
 inline void initMaxAndShift(DWORD mask, int* max, int* shift) {
   for ((*shift) = 0; (mask & 1) == 0; (*shift)++) mask >>= 1;
   (*max) = (rdr::U16)mask;
 }
 
-void DIBSectionBuffer::recreateBuffer() {
+void DIBSectionBuffer::setSize(int w, int h) {
   HBITMAP new_bitmap = 0;
   rdr::U8* new_data = 0;
 
-  if (width_ && height_ && (format.depth != 0)) {
+  if (w && h && (format.depth != 0)) {
     BitmapInfo bi;
     memset(&bi, 0, sizeof(bi));
     UINT iUsage = DIB_RGB_COLORS;
     bi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
     bi.bmiHeader.biBitCount = format.bpp;
-    bi.bmiHeader.biSizeImage = (format.bpp / 8) * width_ * height_;
+    bi.bmiHeader.biSizeImage = (format.bpp / 8) * w * h;
     bi.bmiHeader.biPlanes = 1;
-    bi.bmiHeader.biWidth = width_;
-    bi.bmiHeader.biHeight = -height_;
+    bi.bmiHeader.biWidth = w;
+    bi.bmiHeader.biHeight = -h;
     bi.bmiHeader.biCompression = (format.bpp > 8) ? BI_BITFIELDS : BI_RGB;
     bi.mask.red = format.pixelFromRGB((rdr::U16)~0, 0, 0);
     bi.mask.green = format.pixelFromRGB(0, (rdr::U16)~0, 0);
@@ -115,12 +104,12 @@ void DIBSectionBuffer::recreateBuffer()
     if (device) {
       BitmapDC src_dev(device, bitmap);
       BitmapDC dest_dev(device, new_bitmap);
-      BitBlt(dest_dev, 0, 0, width_, height_, src_dev, 0, 0, SRCCOPY);
+      BitBlt(dest_dev, 0, 0, w, h, src_dev, 0, 0, SRCCOPY);
     } else {
       WindowDC wndDC(window);
       BitmapDC src_dev(wndDC, bitmap);
       BitmapDC dest_dev(wndDC, new_bitmap);
-      BitBlt(dest_dev, 0, 0, width_, height_, src_dev, 0, 0, SRCCOPY);
+      BitBlt(dest_dev, 0, 0, w, h, src_dev, 0, 0, SRCCOPY);
     }
   }
   
@@ -128,17 +117,17 @@ void DIBSectionBuffer::recreateBuffer()
     // Delete the old bitmap
     DeleteObject(bitmap);
     bitmap = 0;
-    data = 0;
+    setBuffer(0, 0, NULL, 0);
   }
 
   if (new_bitmap) {
     int bpp, depth;
     int redMax, greenMax, blueMax;
     int redShift, greenShift, blueShift;
+    int new_stride;
 
     // Set up the new bitmap
     bitmap = new_bitmap;
-    data = new_data;
 
     // Determine the *actual* DIBSection format
     DIBSECTION ds;
@@ -147,14 +136,16 @@ void DIBSectionBuffer::recreateBuffer()
 
     // Correct the "stride" of the DIB
     // *** This code DWORD aligns each row - is that right???
-    stride = width_;
-    int bytesPerRow = stride * format.bpp/8;
+    new_stride = w;
+    int bytesPerRow = new_stride * format.bpp/8;
     if (bytesPerRow % 4) {
       bytesPerRow += 4 - (bytesPerRow % 4);
-      stride = (bytesPerRow * 8) / format.bpp;
-      vlog.info("adjusting DIB stride: %d to %d", width_, stride);
+      new_stride = (bytesPerRow * 8) / format.bpp;
+      vlog.info("adjusting DIB stride: %d to %d", w, new_stride);
     }
 
+    setBuffer(w, h, new_data, new_stride);
+
     // Calculate the PixelFormat for the DIB
     bpp = depth = ds.dsBm.bmBitsPixel;

Places

File 0002-Encapsulate-PixelBuffer-internal-details.patch of Package tigervnc.20176

Places