From 9312b0e3e16a0eee66945a1220d914067132de9a Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Wed, 20 Jun 2018 12:25:14 +0200 Subject: [PATCH] Encapsulate screen layout storage in ConnParams Avoid direct access to the screen dimensions and layout so that we can make sure it stays sane. This also makes sure the layout is properly updated when we only get the screen dimensions from the server. --- common/rfb/CMsgHandler.cxx | 10 ++------- common/rfb/CMsgReader.cxx | 4 ++-- common/rfb/CMsgWriter.cxx | 4 ++-- common/rfb/ConnParams.cxx | 22 +++++++++++++++++-- common/rfb/ConnParams.h | 12 ++++++++--- common/rfb/SMsgHandler.cxx | 4 +--- common/rfb/SMsgWriter.cxx | 10 ++++----- common/rfb/VNCSConnectionST.cxx | 38 +++++++++++++++++---------------- tests/decperf.cxx | 2 +- tests/encperf.cxx | 2 +- vncviewer/CConn.cxx | 15 +++++++------ vncviewer/DesktopWindow.cxx | 24 ++++++++++----------- 12 files changed, 84 insertions(+), 63 deletions(-) diff --git a/common/rfb/CMsgHandler.cxx b/common/rfb/CMsgHandler.cxx index b89bc184..03e66e83 100644 --- a/common/rfb/CMsgHandler.cxx +++ b/common/rfb/CMsgHandler.cxx @@ -34,8 +34,7 @@ CMsgHandler::~CMsgHandler() void CMsgHandler::setDesktopSize(int width, int height) { - cp.width = width; - cp.height = height; + cp.setDimensions(width, height); } void CMsgHandler::setExtendedDesktopSize(unsigned reason, unsigned result, @@ -47,12 +46,7 @@ void CMsgHandler::setExtendedDesktopSize(unsigned reason, unsigned result, if ((reason == reasonClient) && (result != resultSuccess)) return; - if (!layout.validate(width, height)) - fprintf(stderr, "Server sent us an invalid screen layout\n"); - - cp.width = width; - cp.height = height; - cp.screenLayout = layout; + cp.setDimensions(width, height, layout); } void CMsgHandler::setPixelFormat(const PixelFormat& pf) diff --git a/common/rfb/CMsgReader.cxx b/common/rfb/CMsgReader.cxx index 1d359d2c..7502df6a 100644 --- a/common/rfb/CMsgReader.cxx +++ b/common/rfb/CMsgReader.cxx @@ -192,10 +192,10 @@ void CMsgReader::readFramebufferUpdate() void CMsgReader::readRect(const Rect& r, int encoding) { - if ((r.br.x > handler->cp.width) || (r.br.y > handler->cp.height)) { + if ((r.br.x > handler->cp.width()) || (r.br.y > handler->cp.height())) { fprintf(stderr, "Rect too big: %dx%d at %d,%d exceeds %dx%d\n", r.width(), r.height(), r.tl.x, r.tl.y, - handler->cp.width, handler->cp.height); + handler->cp.width(), handler->cp.height()); throw Exception("Rect too big"); } diff --git a/common/rfb/CMsgWriter.cxx b/common/rfb/CMsgWriter.cxx index 44b73dab..97c33363 100644 --- a/common/rfb/CMsgWriter.cxx +++ b/common/rfb/CMsgWriter.cxx @@ -245,8 +245,8 @@ void CMsgWriter::writePointerEvent(const Point& pos, int buttonMask) Point p(pos); if (p.x < 0) p.x = 0; if (p.y < 0) p.y = 0; - if (p.x >= cp->width) p.x = cp->width - 1; - if (p.y >= cp->height) p.y = cp->height - 1; + if (p.x >= cp->width()) p.x = cp->width() - 1; + if (p.y >= cp->height()) p.y = cp->height() - 1; startMsg(msgTypePointerEvent); os->writeU8(buttonMask); diff --git a/common/rfb/ConnParams.cxx b/common/rfb/ConnParams.cxx index 405a99cc..1fdf8f38 100644 --- a/common/rfb/ConnParams.cxx +++ b/common/rfb/ConnParams.cxx @@ -28,7 +28,7 @@ using namespace rfb; ConnParams::ConnParams() : majorVersion(0), minorVersion(0), - width(0), height(0), useCopyRect(false), + useCopyRect(false), supportsLocalCursor(false), supportsLocalXCursor(false), supportsLocalCursorWithAlpha(false), supportsDesktopResize(false), supportsExtendedDesktopSize(false), @@ -37,7 +37,8 @@ ConnParams::ConnParams() supportsSetDesktopSize(false), supportsFence(false), supportsContinuousUpdates(false), compressLevel(2), qualityLevel(-1), fineQualityLevel(-1), - subsampling(subsampleUndefined), name_(0), + subsampling(subsampleUndefined), + width_(0), height_(0), name_(0), ledState_(ledUnknown) { setName(""); @@ -50,6 +51,23 @@ ConnParams::~ConnParams() delete cursor_; } +void ConnParams::setDimensions(int width, int height) +{ + ScreenSet layout; + layout.add_screen(rfb::Screen(0, 0, 0, width, height, 0)); + setDimensions(width, height, layout); +} + +void ConnParams::setDimensions(int width, int height, const ScreenSet& layout) +{ + if (!layout.validate(width, height)) + throw Exception("Attempted to configure an invalid screen layout"); + + width_ = width; + height_ = height; + screenLayout_ = layout; +} + void ConnParams::setPF(const PixelFormat& pf) { pf_ = pf; diff --git a/common/rfb/ConnParams.h b/common/rfb/ConnParams.h index b56c9407..1640efcc 100644 --- a/common/rfb/ConnParams.h +++ b/common/rfb/ConnParams.h @@ -62,9 +62,11 @@ namespace rfb { return !beforeVersion(major,minor+1); } - int width; - int height; - ScreenSet screenLayout; + const int width() const { return width_; } + const int height() const { return height_; } + const ScreenSet& screenLayout() const { return screenLayout_; } + void setDimensions(int width, int height); + void setDimensions(int width, int height, const ScreenSet& layout); const PixelFormat& pf() const { return pf_; } void setPF(const PixelFormat& pf); @@ -105,6 +107,10 @@ namespace rfb { private: + int width_; + int height_; + ScreenSet screenLayout_; + PixelFormat pf_; char* name_; Cursor* cursor_; diff --git a/common/rfb/SMsgHandler.cxx b/common/rfb/SMsgHandler.cxx index c38458c3..137734b3 100644 --- a/common/rfb/SMsgHandler.cxx +++ b/common/rfb/SMsgHandler.cxx @@ -86,8 +86,6 @@ void SMsgHandler::supportsQEMUKeyEvent() void SMsgHandler::setDesktopSize(int fb_width, int fb_height, const ScreenSet& layout) { - cp.width = fb_width; - cp.height = fb_height; - cp.screenLayout = layout; + cp.setDimensions(fb_width, fb_height, layout); } diff --git a/common/rfb/SMsgWriter.cxx b/common/rfb/SMsgWriter.cxx index 3da9413f..96df6533 100644 --- a/common/rfb/SMsgWriter.cxx +++ b/common/rfb/SMsgWriter.cxx @@ -49,8 +49,8 @@ SMsgWriter::~SMsgWriter() void SMsgWriter::writeServerInit() { - os->writeU16(cp->width); - os->writeU16(cp->height); + os->writeU16(cp->width()); + os->writeU16(cp->height()); cp->pf().write(os); os->writeString(cp->name()); endMsg(); @@ -422,15 +422,15 @@ void SMsgWriter::writeNoDataRects() // Send this before SetDesktopSize to make life easier on the clients if (needExtendedDesktopSize) { - writeExtendedDesktopSizeRect(0, 0, cp->width, cp->height, - cp->screenLayout); + writeExtendedDesktopSizeRect(0, 0, cp->width(), cp->height(), + cp->screenLayout()); needExtendedDesktopSize = false; } // Some clients assume this is the last rectangle so don't send anything // more after this if (needSetDesktopSize) { - writeSetDesktopSizeRect(cp->width, cp->height); + writeSetDesktopSizeRect(cp->width(), cp->height()); needSetDesktopSize = false; } } diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx index f1591f4c..4dd00354 100644 --- a/common/rfb/VNCSConnectionST.cxx +++ b/common/rfb/VNCSConnectionST.cxx @@ -192,8 +192,9 @@ void VNCSConnectionST::pixelBufferChange() { try { if (!authenticated()) return; - if (cp.width && cp.height && (server->pb->width() != cp.width || - server->pb->height() != cp.height)) + if (cp.width() && cp.height() && + (server->pb->width() != cp.width() || + server->pb->height() != cp.height())) { // We need to clip the next update to the new size, but also add any // extra bits if it's bigger. If we wanted to do this exactly, something @@ -203,18 +204,17 @@ void VNCSConnectionST::pixelBufferChange() //updates.intersect(server->pb->getRect()); // - //if (server->pb->width() > cp.width) - // updates.add_changed(Rect(cp.width, 0, server->pb->width(), + //if (server->pb->width() > cp.width()) + // updates.add_changed(Rect(cp.width(), 0, server->pb->width(), // server->pb->height())); - //if (server->pb->height() > cp.height) - // updates.add_changed(Rect(0, cp.height, cp.width, + //if (server->pb->height() > cp.height()) + // updates.add_changed(Rect(0, cp.height(), client.width(), // server->pb->height())); damagedCursorRegion.assign_intersect(server->pb->getRect()); - cp.width = server->pb->width(); - cp.height = server->pb->height(); - cp.screenLayout = server->screenLayout; + cp.setDimensions(server->pb->width(), server->pb->height(), + server->screenLayout); if (state() == RFBSTATE_NORMAL) { // We should only send EDS to client asking for both if (!writer()->writeExtendedDesktopSize()) { @@ -417,9 +417,8 @@ void VNCSConnectionST::authSuccess() server->startDesktop(); // - Set the connection parameters appropriately - cp.width = server->pb->width(); - cp.height = server->pb->height(); - cp.screenLayout = server->screenLayout; + cp.setDimensions(server->pb->width(), server->pb->height(), + server->screenLayout); cp.setName(server->getName()); cp.setLEDState(server->ledState); @@ -678,10 +677,11 @@ void VNCSConnectionST::framebufferUpdateRequest(const Rect& r,bool incremental) SConnection::framebufferUpdateRequest(r, incremental); // Check that the client isn't sending crappy requests - if (!r.enclosed_by(Rect(0, 0, cp.width, cp.height))) { + if (!r.enclosed_by(Rect(0, 0, cp.width(), cp.height()))) { vlog.error("FramebufferUpdateRequest %dx%d at %d,%d exceeds framebuffer %dx%d", - r.width(), r.height(), r.tl.x, r.tl.y, cp.width, cp.height); - safeRect = r.intersect(Rect(0, 0, cp.width, cp.height)); + r.width(), r.height(), r.tl.x, r.tl.y, + cp.width(), cp.height()); + safeRect = r.intersect(Rect(0, 0, cp.width(), cp.height())); } else { safeRect = r; } @@ -1124,13 +1124,15 @@ void VNCSConnectionST::screenLayoutChange(rdr::U16 reason) if (!authenticated()) return; - cp.screenLayout = server->screenLayout; + cp.setDimensions(cp.width(), cp.height(), + server->screenLayout); if (state() != RFBSTATE_NORMAL) return; - writer()->writeExtendedDesktopSize(reason, 0, cp.width, cp.height, - cp.screenLayout); + writer()->writeExtendedDesktopSize(reason, 0, + cp.width(), cp.height(), + cp.screenLayout()); } diff --git a/tests/decperf.cxx b/tests/decperf.cxx index 9061cb53..24f66042 100644 --- a/tests/decperf.cxx +++ b/tests/decperf.cxx @@ -85,7 +85,7 @@ void CConn::setDesktopSize(int w, int h) { CConnection::setDesktopSize(w, h); - setFramebuffer(new rfb::ManagedPixelBuffer(filePF, cp.width, cp.height)); + setFramebuffer(new rfb::ManagedPixelBuffer(filePF, cp.width(), cp.height())); } void CConn::setPixelFormat(const rfb::PixelFormat& pf) diff --git a/tests/encperf.cxx b/tests/encperf.cxx index 4e7038fd..a232fb92 100644 --- a/tests/encperf.cxx +++ b/tests/encperf.cxx @@ -203,7 +203,7 @@ void CConn::setDesktopSize(int w, int h) CConnection::setDesktopSize(w, h); pb = new rfb::ManagedPixelBuffer((bool)translate ? fbPF : cp.pf(), - cp.width, cp.height); + cp.width(), cp.height()); setFramebuffer(pb); } diff --git a/vncviewer/CConn.cxx b/vncviewer/CConn.cxx index 69186c55..e87ebcde 100644 --- a/vncviewer/CConn.cxx +++ b/vncviewer/CConn.cxx @@ -191,7 +191,7 @@ const char *CConn::connectionInfo() strcat(infoText, "\n"); snprintf(scratch, sizeof(scratch), - _("Size: %d x %d"), cp.width, cp.height); + _("Size: %d x %d"), cp.width(), cp.height()); strcat(infoText, scratch); strcat(infoText, "\n"); @@ -324,7 +324,8 @@ void CConn::serverInit() serverPF = cp.pf(); - desktop = new DesktopWindow(cp.width, cp.height, cp.name(), serverPF, this); + desktop = new DesktopWindow(cp.width(), cp.height(), + cp.name(), serverPF, this); fullColourPF = desktop->getPreferredPF(); // Force a switch to the format and encoding we'd like @@ -478,7 +479,8 @@ void CConn::fence(rdr::U32 flags, unsigned len, const char data[]) if (cp.supportsContinuousUpdates) { vlog.info(_("Enabling continuous updates")); continuousUpdates = true; - writer()->writeEnableContinuousUpdates(true, 0, 0, cp.width, cp.height); + writer()->writeEnableContinuousUpdates(true, 0, 0, + cp.width(), cp.height()); } } } else { @@ -508,9 +510,10 @@ void CConn::resizeFramebuffer() return; if (continuousUpdates) - writer()->writeEnableContinuousUpdates(true, 0, 0, cp.width, cp.height); + writer()->writeEnableContinuousUpdates(true, 0, 0, + cp.width(), cp.height()); - desktop->resizeFramebuffer(cp.width, cp.height); + desktop->resizeFramebuffer(cp.width(), cp.height()); } // autoSelectFormatAndEncoding() chooses the format and encoding appropriate @@ -647,7 +650,7 @@ void CConn::requestNewUpdate() if (forceNonincremental || !continuousUpdates) { pendingUpdate = true; - writer()->writeFramebufferUpdateRequest(Rect(0, 0, cp.width, cp.height), + writer()->writeFramebufferUpdateRequest(Rect(0, 0, cp.width(), cp.height()), !forceNonincremental); } diff --git a/vncviewer/DesktopWindow.cxx b/vncviewer/DesktopWindow.cxx index d070b648..e6d0ca7c 100644 --- a/vncviewer/DesktopWindow.cxx +++ b/vncviewer/DesktopWindow.cxx @@ -1014,14 +1014,14 @@ void DesktopWindow::handleResizeTimeout(void *data) void DesktopWindow::remoteResize(int width, int height) { ScreenSet layout; - ScreenSet::iterator iter; + ScreenSet::const_iterator iter; if (!fullscreen_active() || (width > w()) || (height > h())) { // In windowed mode (or the framebuffer is so large that we need // to scroll) we just report a single virtual screen that covers // the entire framebuffer. - layout = cc->cp.screenLayout; + layout = cc->cp.screenLayout(); // Not sure why we have no screens, but adding a new one should be // safe as there is nothing to conflict with... @@ -1077,8 +1077,8 @@ void DesktopWindow::remoteResize(int width, int height) sy -= viewport_rect.tl.y; // Look for perfectly matching existing screen... - for (iter = cc->cp.screenLayout.begin(); - iter != cc->cp.screenLayout.end(); ++iter) { + for (iter = cc->cp.screenLayout().begin(); + iter != cc->cp.screenLayout().end(); ++iter) { if ((iter->dimensions.tl.x == sx) && (iter->dimensions.tl.y == sy) && (iter->dimensions.width() == sw) && @@ -1087,7 +1087,7 @@ void DesktopWindow::remoteResize(int width, int height) } // Found it? - if (iter != cc->cp.screenLayout.end()) { + if (iter != cc->cp.screenLayout().end()) { layout.add_screen(*iter); continue; } @@ -1095,13 +1095,13 @@ void DesktopWindow::remoteResize(int width, int height) // Need to add a new one, which means we need to find an unused id while (true) { id = rand(); - for (iter = cc->cp.screenLayout.begin(); - iter != cc->cp.screenLayout.end(); ++iter) { + for (iter = cc->cp.screenLayout().begin(); + iter != cc->cp.screenLayout().end(); ++iter) { if (iter->id == id) break; } - if (iter == cc->cp.screenLayout.end()) + if (iter == cc->cp.screenLayout().end()) break; } @@ -1115,14 +1115,14 @@ void DesktopWindow::remoteResize(int width, int height) } // Do we actually change anything? - if ((width == cc->cp.width) && - (height == cc->cp.height) && - (layout == cc->cp.screenLayout)) + if ((width == cc->cp.width()) && + (height == cc->cp.height()) && + (layout == cc->cp.screenLayout())) return; char buffer[2048]; vlog.debug("Requesting framebuffer resize from %dx%d to %dx%d", - cc->cp.width, cc->cp.height, width, height); + cc->cp.width(), cc->cp.height(), width, height); layout.print(buffer, sizeof(buffer)); vlog.debug("%s", buffer); -- 2.39.5