xcb: respect big-request encoding in max request size

From big-request specification:

"This extension defines a mechanism for extending the length
field beyond 16 bits. If the normal 16-bit length field of the protocol
request is zero, then an additional 32-bit field containing the actual
length (in 4-byte units) is inserted into the request, immediately
following the 16-bit length field."

Meaning that the request requires 4 additional bytes. This patch provides
a convenience API for calculating maximum request data size.

Besides fixing QTBUG-73044, it was also discovered that calculations
for xcb_image_put (in QXcbBackingStoreImage::flushPixmap) were wrong.
The code assumed that xcb_get_maximum_request_length() returns bytes,
but what it actually returns is length which is measured in four-byte
units. This means that we were sending 4x less bytes than allowed by
the protocol. Furthermore, use the actual 'stride' (bytes per line) value
when calculating rows_per_put. The new stride value was introduced
by 760b2929a3b268e2edf14a561329bdb78fbdc26e, but was not updated in
rows_per_put calculations.

Fixes: QTBUG-73044
Done-with: JiDe Zhang <zccrs@live.com>
Done-with: Mikhail Svetkin <mikhail.svetkin@qt.io>
Change-Id: I06beb6082da3e8bc78225a87603914e796fe5878
Reviewed-by: Błażej Szczygieł <spaz16@wp.pl>
Reviewed-by: JiDe Zhang <zccrs@live.com>
Reviewed-by: Mikhail Svetkin <mikhail.svetkin@qt.io>
Reviewed-by: Uli Schlachter <psychon@znc.in>
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

6 files changed, 39 insertions, 26 deletions

diff --git a/src/plugins/platforms/xcb/qxcbbackingstore.cpp b/src/plugins/platforms/xcb/qxcbbackingstore.cpp
index f9240a45cc..741317d766 100644
--- a/src/plugins/platforms/xcb/qxcbbackingstore.cpp
+++ b/src/plugins/platforms/xcb/qxcbbackingstore.cpp
@@ -641,17 +641,17 @@ void QXcbBackingStoreImage::flushPixmap(const QRegion &region, bool fullRegion)
     xcb_subimage.bit_order = m_xcb_image->bit_order;
 
     const bool needsByteSwap = xcb_subimage.byte_order != m_xcb_image->byte_order;
+    // Ensure that we don't send more than maxPutImageRequestDataBytes per request.
+    const auto maxPutImageRequestDataBytes = connection()->maxRequestDataBytes(sizeof(xcb_put_image_request_t));
 
     for (const QRect &rect : region) {
-        // We must make sure that each request is not larger than max_req_size.
-        // Each request takes req_size + m_xcb_image->stride * height bytes.
-        static const uint32_t req_size = sizeof(xcb_put_image_request_t);
-        const uint32_t max_req_size = xcb_get_maximum_request_length(xcb_connection());
-        const int rows_per_put = (max_req_size - req_size) / m_xcb_image->stride;
+        const quint32 stride = round_up_scanline(rect.width() * m_qimage.depth(), xcb_subimage.scanline_pad) >> 3;
+        const int rows_per_put = maxPutImageRequestDataBytes / stride;
 
         // This assert could trigger if a single row has more pixels than fit in
-        // a single PutImage request. However, max_req_size is guaranteed to be
-        // at least 16384 bytes. That should be enough for quite large images.
+        // a single PutImage request. In the absence of the BIG-REQUESTS extension
+        // the theoretical maximum lengths of maxPutImageRequestDataBytes can be
+        // roughly 256kB.
         Q_ASSERT(rows_per_put > 0);
 
         // If we upload the whole image in a single chunk, the result might be
@@ -666,9 +666,10 @@ void QXcbBackingStoreImage::flushPixmap(const QRegion &region, bool fullRegion)
         while (height > 0) {
             const int rows = std::min(height, rows_per_put);
             const QRect subRect(x, y, width, rows);
-            const quint32 stride = round_up_scanline(width * m_qimage.depth(), xcb_subimage.scanline_pad) >> 3;
             const QImage subImage = native_sub_image(&m_flushBuffer, stride, m_qimage, subRect, needsByteSwap);
 
+            Q_ASSERT(static_cast<size_t>(subImage.sizeInBytes()) <= maxPutImageRequestDataBytes);
+
             xcb_subimage.width = width;
             xcb_subimage.height = rows;
             xcb_subimage.data = const_cast<uint8_t *>(subImage.constBits());
diff --git a/src/plugins/platforms/xcb/qxcbclipboard.cpp b/src/plugins/platforms/xcb/qxcbclipboard.cpp
index ac8b029916..2cb6720d40 100644
--- a/src/plugins/platforms/xcb/qxcbclipboard.cpp
+++ b/src/plugins/platforms/xcb/qxcbclipboard.cpp
@@ -240,8 +240,8 @@ QXcbClipboard::QXcbClipboard(QXcbConnection *c)
         xcb_xfixes_select_selection_input_checked(xcb_connection(), m_owner, atom(QXcbAtom::CLIPBOARD), mask);
     }
 
-    // change property protocol request is 24 bytes
-    m_increment = (xcb_get_maximum_request_length(xcb_connection()) * 4) - 24;
+    // xcb_change_property_request_t and xcb_get_property_request_t are the same size
+    m_maxPropertyRequestDataBytes = connection()->maxRequestDataBytes(sizeof(xcb_change_property_request_t));
 }
 
 QXcbClipboard::~QXcbClipboard()
@@ -486,7 +486,7 @@ xcb_atom_t QXcbClipboard::sendSelection(QMimeData *d, xcb_atom_t target, xcb_win
         if (m_clipboard_closing)
             allow_incr = false;
 
-        if (data.size() > m_increment && allow_incr) {
+        if (data.size() > m_maxPropertyRequestDataBytes && allow_incr) {
             long bytes = data.size();
             xcb_change_property(xcb_connection(), XCB_PROP_MODE_REPLACE, window, property,
                                 atom(QXcbAtom::INCR), 32, 1, (const void *)&bytes);
@@ -496,7 +496,7 @@ xcb_atom_t QXcbClipboard::sendSelection(QMimeData *d, xcb_atom_t target, xcb_win
         }
 
         // make sure we can perform the XChangeProperty in a single request
-        if (data.size() > m_increment)
+        if (data.size() > m_maxPropertyRequestDataBytes)
             return XCB_NONE; // ### perhaps use several XChangeProperty calls w/ PropModeAppend?
         int dataSize = data.size() / (dataFormat / 8);
         // use a single request to transfer data
@@ -678,17 +678,8 @@ void QXcbClipboard::handleXFixesSelectionRequest(xcb_xfixes_selection_notify_eve
         emitChanged(mode);
 }
 
-
-static inline int maxSelectionIncr(xcb_connection_t *c)
-{
-    int l = xcb_get_maximum_request_length(c);
-    return (l > 65536 ? 65536*4 : l*4) - 100;
-}
-
 bool QXcbClipboard::clipboardReadProperty(xcb_window_t win, xcb_atom_t property, bool deleteProperty, QByteArray *buffer, int *size, xcb_atom_t *type, int *format)
 {
-    int    maxsize = maxSelectionIncr(xcb_connection());
-    ulong  bytes_left; // bytes_after
     xcb_atom_t   dummy_type;
     int    dummy_format;
 
@@ -705,7 +696,8 @@ bool QXcbClipboard::clipboardReadProperty(xcb_window_t win, xcb_atom_t property,
     }
     *type = reply->type;
     *format = reply->format;
-    bytes_left = reply->bytes_after;
+
+    auto bytes_left = reply->bytes_after;
 
     int  offset = 0, buffer_offset = 0;
 
@@ -720,7 +712,8 @@ bool QXcbClipboard::clipboardReadProperty(xcb_window_t win, xcb_atom_t property,
         while (bytes_left) {
             // more to read...
 
-            reply = Q_XCB_REPLY(xcb_get_property, xcb_connection(), false, win, property, XCB_GET_PROPERTY_TYPE_ANY, offset, maxsize/4);
+            reply = Q_XCB_REPLY(xcb_get_property, xcb_connection(), false, win, property,
+                                XCB_GET_PROPERTY_TYPE_ANY, offset, m_maxPropertyRequestDataBytes / 4);
             if (!reply || reply->type == XCB_NONE)
                 break;
 
diff --git a/src/plugins/platforms/xcb/qxcbclipboard.h b/src/plugins/platforms/xcb/qxcbclipboard.h
index 26d3b3b395..51ae0dc1ee 100644
--- a/src/plugins/platforms/xcb/qxcbclipboard.h
+++ b/src/plugins/platforms/xcb/qxcbclipboard.h
@@ -113,7 +113,7 @@ public:
     xcb_window_t getSelectionOwner(xcb_atom_t atom) const;
     QByteArray getSelection(xcb_atom_t selection, xcb_atom_t target, xcb_atom_t property, xcb_timestamp_t t = 0);
 
-    int increment() const { return m_increment; }
+    int increment() const { return m_maxPropertyRequestDataBytes; }
     int clipboardTimeout() const { return clipboard_timeout; }
 
     void removeTransaction(xcb_window_t window) { m_transactions.remove(window); }
@@ -137,7 +137,7 @@ private:
 
     static const int clipboard_timeout;
 
-    int m_increment = 0;
+    int m_maxPropertyRequestDataBytes = 0;
     bool m_clipboard_closing = false;
     xcb_timestamp_t m_incr_receive_time = 0;
 
diff --git a/src/plugins/platforms/xcb/qxcbconnection.h b/src/plugins/platforms/xcb/qxcbconnection.h
index d63888b48f..0e3ecd135b 100644
--- a/src/plugins/platforms/xcb/qxcbconnection.h
+++ b/src/plugins/platforms/xcb/qxcbconnection.h
@@ -245,7 +245,7 @@ public:
     void flush() { xcb_flush(xcb_connection()); }
     void processXcbEvents(QEventLoop::ProcessEventsFlags flags);
 
-    QTimer &focusInTimer() { return m_focusInTimer; };
+    QTimer &focusInTimer() { return m_focusInTimer; }
 
 protected:
     bool event(QEvent *e) override;
diff --git a/src/plugins/platforms/xcb/qxcbconnection_basic.cpp b/src/plugins/platforms/xcb/qxcbconnection_basic.cpp
index af72285135..9a028e5a7e 100644
--- a/src/plugins/platforms/xcb/qxcbconnection_basic.cpp
+++ b/src/plugins/platforms/xcb/qxcbconnection_basic.cpp
@@ -134,6 +134,7 @@ QXcbBasicConnection::QXcbBasicConnection(const char *displayName)
 
     m_setup = xcb_get_setup(m_xcbConnection);
     m_xcbAtom.initialize(m_xcbConnection);
+    m_maximumRequestLength = xcb_get_maximum_request_length(m_xcbConnection);
 
     xcb_extension_t *extensions[] = {
         &xcb_shm_id, &xcb_xfixes_id, &xcb_randr_id, &xcb_shape_id, &xcb_sync_id,
@@ -178,6 +179,14 @@ QXcbBasicConnection::~QXcbBasicConnection()
     }
 }
 
+size_t QXcbBasicConnection::maxRequestDataBytes(size_t requestSize) const
+{
+    if (hasBigRequest())
+        requestSize += 4; // big-request encoding adds 4 bytes
+
+    return m_maximumRequestLength * 4 - requestSize;
+}
+
 xcb_atom_t QXcbBasicConnection::internAtom(const char *name)
 {
     if (!name || *name == 0)
@@ -199,6 +208,11 @@ QByteArray QXcbBasicConnection::atomName(xcb_atom_t atom)
     return QByteArray();
 }
 
+bool QXcbBasicConnection::hasBigRequest() const
+{
+    return m_maximumRequestLength > m_setup->maximum_request_length;
+}
+
 #if QT_CONFIG(xcb_xinput)
 // Starting from the xcb version 1.9.3 struct xcb_ge_event_t has changed:
 // - "pad0" became "extension"
diff --git a/src/plugins/platforms/xcb/qxcbconnection_basic.h b/src/plugins/platforms/xcb/qxcbconnection_basic.h
index ca91f7fa45..3691763398 100644
--- a/src/plugins/platforms/xcb/qxcbconnection_basic.h
+++ b/src/plugins/platforms/xcb/qxcbconnection_basic.h
@@ -73,6 +73,8 @@ public:
     }
     const xcb_setup_t *setup() const { return m_setup; }
 
+    size_t maxRequestDataBytes(size_t requestSize) const;
+
     inline xcb_atom_t atom(QXcbAtom::Atom qatom) const { return m_xcbAtom.atom(qatom); }
     QXcbAtom::Atom qatom(xcb_atom_t atom) const { return m_xcbAtom.qatom(atom); }
     xcb_atom_t internAtom(const char *name);
@@ -94,6 +96,7 @@ public:
     bool hasShmFd() const { return m_hasShmFd; }
     bool hasXSync() const { return m_hasXSync; }
     bool hasXinerama() const { return m_hasXinerama; }
+    bool hasBigRequest() const;
 
 #if QT_CONFIG(xcb_xinput)
     bool isAtLeastXI21() const { return m_xi2Enabled && m_xi2Minor >= 1; }
@@ -152,6 +155,8 @@ private:
     uint32_t m_xfixesFirstEvent = 0;
     uint32_t m_xrandrFirstEvent = 0;
     uint32_t m_xkbFirstEvent = 0;
+
+    uint32_t m_maximumRequestLength = 0;
 };
 
 #define Q_XCB_REPLY_CONNECTION_ARG(connection, ...) connection