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.

(cherry picked from commit 53f913a761)

common/rfb/Cursor.cxx

@@ -272,8 +272,7 @@ void RenderedCursor::update(PixelBuffer* framebuffer,
   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())

common/rfb/EncodeManager.cxx

@@ -1049,11 +1049,8 @@ void EncodeManager::OffsetPixelBuffer::update(const PixelFormat& pf,
                                               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

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, const Rect& r, int outStride) const
   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 PixelFormat& pf, void* imageBuf,
   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 PixelFormat& pf, void* imageBuf,
                       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(const Rect& r, const void* pix)
 
   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(const Rect& r,
   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;
 
@@ -214,13 +226,13 @@ void ModifiablePixelBuffer::copyRect(const Rect &rect,
   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());
 
   bytesPerPixel = format.bpp/8;
 
@@ -275,7 +287,7 @@ void ModifiablePixelBuffer::imageRect(const PixelFormat& pf, const Rect &dest,
   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();
@@ -304,7 +316,7 @@ rdr::U8* FullFramePixelBuffer::getBufferRW(const Rect& r, int* stride_)
   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)];
@@ -319,55 +331,68 @@ const rdr::U8* FullFramePixelBuffer::getBuffer(const Rect& r, int* stride_) cons
   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);
+}
 
+ManagedPixelBuffer::~ManagedPixelBuffer()
+{
+  if (data_)
+    delete [] data_;
+}
 
-void
-ManagedPixelBuffer::setPF(const PixelFormat &pf) {
-  format = pf; checkDataSize();
-};
-void
-ManagedPixelBuffer::setSize(int w, int h) {
-  width_ = w; height_ = h; stride = w; checkDataSize();
-};
+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);
+}

common/rfb/PixelBuffer.h

@@ -90,7 +90,12 @@ namespace rfb {
 
   protected:
     PixelBuffer();
+    virtual void setSize(int width, int height);
+
+  protected:
     PixelFormat format;
+
+  private:
     int width_, height_;
   };
 
@@ -154,7 +159,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;
   };
@@ -172,12 +182,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();
   };
 
 };

unix/x0vncserver/XPixelBuffer.cxx

@@ -50,13 +50,8 @@ XPixelBuffer::XPixelBuffer(Display *dpy, ImageFactory &factory,
                        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);

unix/xserver/hw/vnc/XserverDesktop.cc

@@ -75,7 +75,7 @@ XserverDesktop::XserverDesktop(int screenIndex_,
                                void* fbptr, int stride)
   : screenIndex(screenIndex_),
     server(0), listeners(listeners_),
-    directFbptr(true),
+    shadowFramebuffer(NULL),
     queryConnectId(0), queryConnectTimer(this)
 {
   format = pf;
@@ -97,8 +97,8 @@ XserverDesktop::~XserverDesktop()
     delete listeners.back();
     listeners.pop_back();
   }
-  if (!directFbptr)
-    delete [] data;
+  if (shadowFramebuffer)
+    delete [] shadowFramebuffer;
   delete server;
 }
 
@@ -116,22 +116,18 @@ void XserverDesktop::setFramebuffer(int w, int h, void* fbptr, int stride_)
 {
   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_);
 
   vncSetGlueContext(screenIndex);
   layout = ::computeScreenLayout(&outputIdMap);
@@ -492,7 +488,7 @@ void XserverDesktop::handleClipboardData(const char* data_)
 
 void XserverDesktop::grabRegion(const rfb::Region& region)
 {
-  if (directFbptr)
+  if (shadowFramebuffer == NULL)
     return;
 
   std::vector<rfb::Rect> rects;

unix/xserver/hw/vnc/XserverDesktop.h

@@ -118,7 +118,7 @@ private:
   int screenIndex;
   rfb::VNCServer* server;
   std::list<network::SocketListener*> listeners;
-  bool directFbptr;
+  rdr::U8* shadowFramebuffer;
 
   uint32_t queryConnectId;
   network::Socket* queryConnectSocket;

vncviewer/PlatformPixelBuffer.cxx

@@ -36,7 +36,7 @@ static rfb::LogWriter vlog("PlatformPixelBuffer");
 PlatformPixelBuffer::PlatformPixelBuffer(int width, int height) :
   FullFramePixelBuffer(rfb::PixelFormat(32, 24, false, true,
                                         255, 255, 255, 16, 8, 0),
-                       width, height, NULL, 0),
+                       0, 0, NULL, 0),
   Surface(width, height)
 #if !defined(WIN32) && !defined(__APPLE__)
   , shminfo(NULL), xim(NULL)
@@ -56,14 +56,13 @@ PlatformPixelBuffer::PlatformPixelBuffer(int width, int height) :
     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));
 
   // On X11, the Pixmap backing this Surface is uninitialized.
   clear(0, 0, 0);
 #else
-  FullFramePixelBuffer::data = (rdr::U8*)Surface::data;
-  stride = width;
+  setBuffer(width, height, (rdr::U8*)Surface::data, width);
 #endif
 }
 

win/rfb_win32/DIBSectionBuffer.cxx

@@ -52,39 +52,28 @@ void DIBSectionBuffer::setPF(const PixelFormat& pf) {
   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;