Skip to content

Commit

Permalink
xcb: fix regression with remote X11 clients
Browse files Browse the repository at this point in the history 
There were several issues here:

We were attempting to use MIT-SHM functions over SSH connection,
which is not supported. X server should detect this and return with
an appropriate error message. It does actually return BadAccess for
non-fd code path, but Qt was stubbornly trying to repeat this action
and always falling back to malloc (during window resizing). For fd
code path we were hitting X server bug, which would result in window
freeze [1].

During the initialization we check if xcb_shm_attach_checked() fails,
and disable MIT-SHM if it does. We use this logic to detect if we
are running remotely, as there are no public APIs for it. This way
we can avoid X server bug and avoid needless calling of code path
which will _always_ fail on a remote X11 connection.

[1] https://lists.x.org/archives/xorg-devel/2018-June/057011.html

Task-number: QTBUG-68449
Task-number: QTBUG-68783
Change-Id: I7ab3dcf0f323fd53001b9f7b88c2cb10809af509
Reviewed-by: Gatis Paeglis <[email protected]>
  • Loading branch information
@gatispaeglis @jaheikk
gatispaeglis authored and jaheikk committed Jun 11, 2018
1 parent 89f9a3d commit 67227ae
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 53 deletions.
107 changes: 62 additions & 45 deletions src/plugins/platforms/xcb/qxcbbackingstore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class QXcbBackingStoreImage : public QXcbObject
void put(xcb_drawable_t dst, const QRegion &region, const QPoint &offset);
void preparePaint(const QRegion &region);

static bool createSystemVShmSegment(QXcbConnection *c, size_t segmentSize = 1,
xcb_shm_segment_info_t *shm_info = nullptr);

private:
void createShmSegment(size_t segmentSize);
void destroyShmSegment(size_t segmentSize);
Expand Down Expand Up @@ -325,15 +328,16 @@ void QXcbBackingStoreImage::createShmSegment(size_t segmentSize)
#ifdef XCB_USE_SHM_FD
if (connection()->hasShmFd()) {
if (Q_UNLIKELY(segmentSize > std::numeric_limits<uint32_t>::max())) {
qWarning("QXcbShmImage: xcb_shm_create_segment() can't be called for size %zu, maximum allowed size is %u",
segmentSize, std::numeric_limits<uint32_t>::max());
qCWarning(lcQpaXcb, "xcb_shm_create_segment() can't be called for size %zu, maximum"
"allowed size is %u", segmentSize, std::numeric_limits<uint32_t>::max());
return;
}

const auto seg = xcb_generate_id(xcb_connection());
auto reply = Q_XCB_REPLY(xcb_shm_create_segment,
xcb_connection(), seg, segmentSize, false);
if (!reply) {
qWarning("QXcbShmImage: xcb_shm_create_segment() failed for size %zu", segmentSize);
qCWarning(lcQpaXcb, "xcb_shm_create_segment() failed for size %zu", segmentSize);
return;
}

Expand All @@ -342,13 +346,13 @@ void QXcbBackingStoreImage::createShmSegment(size_t segmentSize)
for (int i = 0; i < reply->nfd; i++)
close(fds[i]);

qWarning("QXcbShmImage: failed to get file descriptor for shm segment of size %zu", segmentSize);
qCWarning(lcQpaXcb, "failed to get file descriptor for shm segment of size %zu", segmentSize);
return;
}

void *addr = mmap(nullptr, segmentSize, PROT_READ|PROT_WRITE, MAP_SHARED, fds[0], 0);
if (addr == MAP_FAILED) {
qWarning("QXcbShmImage: failed to mmap segment from X server (%d: %s) for size %zu",
qCWarning(lcQpaXcb, "failed to mmap segment from X server (%d: %s) for size %zu",
errno, strerror(errno), segmentSize);
close(fds[0]);
xcb_shm_detach(xcb_connection(), seg);
Expand All @@ -358,47 +362,54 @@ void QXcbBackingStoreImage::createShmSegment(size_t segmentSize)
close(fds[0]);
m_shm_info.shmseg = seg;
m_shm_info.shmaddr = static_cast<quint8 *>(addr);

m_segmentSize = segmentSize;
} else
#endif
{
const int id = shmget(IPC_PRIVATE, segmentSize, IPC_CREAT | 0600);
if (id == -1) {
qWarning("QXcbShmImage: shmget() failed (%d: %s) for size %zu",
errno, strerror(errno), segmentSize);
return;
}

void *addr = shmat(id, 0, 0);
if (addr == (void *)-1) {
qWarning("QXcbShmImage: shmat() failed (%d: %s) for id %d",
errno, strerror(errno), id);
return;
}

if (shmctl(id, IPC_RMID, 0) == -1)
qWarning("QXcbBackingStore: Error while marking the shared memory segment to be destroyed");
if (createSystemVShmSegment(connection(), segmentSize, &m_shm_info))
m_segmentSize = segmentSize;
}
}

const auto seg = xcb_generate_id(xcb_connection());
auto cookie = xcb_shm_attach_checked(xcb_connection(), seg, id, false);
auto *error = xcb_request_check(xcb_connection(), cookie);
if (error) {
connection()->printXcbError("QXcbShmImage: xcb_shm_attach() failed with error", error);
free(error);
if (shmdt(addr) == -1) {
qWarning("QXcbShmImage: shmdt() failed (%d: %s) for %p",
errno, strerror(errno), addr);
}
return;
}
bool QXcbBackingStoreImage::createSystemVShmSegment(QXcbConnection *c, size_t segmentSize,
xcb_shm_segment_info_t *shmInfo)
{
const int id = shmget(IPC_PRIVATE, segmentSize, IPC_CREAT | 0600);
if (id == -1) {
qCWarning(lcQpaXcb, "shmget() failed (%d: %s) for size %zu", errno, strerror(errno), segmentSize);
return false;
}

m_shm_info.shmseg = seg;
m_shm_info.shmid = id; // unused
m_shm_info.shmaddr = static_cast<quint8 *>(addr);
void *addr = shmat(id, 0, 0);
if (addr == (void *)-1) {
qCWarning(lcQpaXcb, "shmat() failed (%d: %s) for id %d", errno, strerror(errno), id);
return false;
}

m_segmentSize = segmentSize;
if (shmctl(id, IPC_RMID, 0) == -1)
qCWarning(lcQpaXcb, "Error while marking the shared memory segment to be destroyed");

const auto seg = xcb_generate_id(c->xcb_connection());
auto cookie = xcb_shm_attach_checked(c->xcb_connection(), seg, id, false);
auto *error = xcb_request_check(c->xcb_connection(), cookie);
if (error) {
c->printXcbError("xcb_shm_attach() failed with error", error);
free(error);
if (shmdt(addr) == -1)
qCWarning(lcQpaXcb, "shmdt() failed (%d: %s) for %p", errno, strerror(errno), addr);
return false;
} else if (!shmInfo) { // this was a test run, free the allocated test segment
xcb_shm_detach(c->xcb_connection(), seg);
auto shmaddr = static_cast<quint8 *>(addr);
if (shmdt(shmaddr) == -1)
qCWarning(lcQpaXcb, "shmdt() failed (%d: %s) for %p", errno, strerror(errno), shmaddr);
}
if (shmInfo) {
shmInfo->shmseg = seg;
shmInfo->shmid = id; // unused
shmInfo->shmaddr = static_cast<quint8 *>(addr);
}
return true;
}

void QXcbBackingStoreImage::destroyShmSegment(size_t segmentSize)
Expand All @@ -409,21 +420,21 @@ void QXcbBackingStoreImage::destroyShmSegment(size_t segmentSize)
auto cookie = xcb_shm_detach_checked(xcb_connection(), m_shm_info.shmseg);
xcb_generic_error_t *error = xcb_request_check(xcb_connection(), cookie);
if (error)
connection()->printXcbError("QXcbShmImage: xcb_shm_detach() failed with error", error);
connection()->printXcbError("xcb_shm_detach() failed with error", error);
m_shm_info.shmseg = 0;

#ifdef XCB_USE_SHM_FD
if (connection()->hasShmFd()) {
if (munmap(m_shm_info.shmaddr, segmentSize) == -1) {
qWarning("QXcbShmImage: munmap() failed (%d: %s) for %p with size %zu",
errno, strerror(errno), m_shm_info.shmaddr, segmentSize);
qCWarning(lcQpaXcb, "munmap() failed (%d: %s) for %p with size %zu",
errno, strerror(errno), m_shm_info.shmaddr, segmentSize);
}
} else
#endif
{
if (shmdt(m_shm_info.shmaddr) == -1) {
qWarning("QXcbShmImage: shmdt() failed (%d: %s) for %p",
errno, strerror(errno), m_shm_info.shmaddr);
qCWarning(lcQpaXcb, "shmdt() failed (%d: %s) for %p",
errno, strerror(errno), m_shm_info.shmaddr);
}
m_shm_info.shmid = 0; // unused
}
Expand Down Expand Up @@ -718,6 +729,12 @@ void QXcbBackingStoreImage::preparePaint(const QRegion &region)
m_pendingFlush |= region;
}

bool QXcbBackingStore::createSystemVShmSegment(QXcbConnection *c, size_t segmentSize, void *shmInfo)
{
auto info = reinterpret_cast<xcb_shm_segment_info_t *>(shmInfo);
return QXcbBackingStoreImage::createSystemVShmSegment(c, segmentSize, info);
}

QXcbBackingStore::QXcbBackingStore(QWindow *window)
: QPlatformBackingStore(window)
{
Expand Down Expand Up @@ -757,7 +774,7 @@ void QXcbBackingStore::beginPaint(const QRegion &region)
void QXcbBackingStore::endPaint()
{
if (Q_UNLIKELY(m_paintRegions.isEmpty())) {
qWarning("%s: paint regions empty!", Q_FUNC_INFO);
qCWarning(lcQpaXcb, "%s: paint regions empty!", Q_FUNC_INFO);
return;
}

Expand Down Expand Up @@ -811,7 +828,7 @@ void QXcbBackingStore::flush(QWindow *window, const QRegion &region, const QPoin

QXcbWindow *platformWindow = static_cast<QXcbWindow *>(window->handle());
if (!platformWindow) {
qWarning("QXcbBackingStore::flush: QWindow has no platform window (QTBUG-32681)");
qCWarning(lcQpaXcb, "%s QWindow has no platform window, see QTBUG-32681", Q_FUNC_INFO);
return;
}

Expand Down
3 changes: 3 additions & 0 deletions src/plugins/platforms/xcb/qxcbbackingstore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ class QXcbBackingStore : public QXcbObject, public QPlatformBackingStore
void beginPaint(const QRegion &) override;
void endPaint() override;

static bool createSystemVShmSegment(QXcbConnection *c, size_t segmentSize = 1,
void *shmInfo = nullptr);

private:
QXcbBackingStoreImage *m_image = nullptr;
QStack<QRegion> m_paintRegions;
Expand Down
31 changes: 23 additions & 8 deletions src/plugins/platforms/xcb/qxcbconnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
#include "qxcbsystemtraytracker.h"
#include "qxcbglintegrationfactory.h"
#include "qxcbglintegration.h"
#include "qxcbbackingstore.h"

#include <QSocketNotifier>
#include <QAbstractEventDispatcher>
Expand Down Expand Up @@ -973,7 +974,7 @@ void QXcbConnection::printXcbError(const char *message, xcb_generic_error_t *err
uint clamped_error_code = qMin<uint>(error->error_code, (sizeof(xcb_errors) / sizeof(xcb_errors[0])) - 1);
uint clamped_major_code = qMin<uint>(error->major_code, (sizeof(xcb_protocol_request_codes) / sizeof(xcb_protocol_request_codes[0])) - 1);

qWarning("%s: %d (%s), sequence: %d, resource id: %d, major code: %d (%s), minor code: %d",
qCWarning(lcQpaXcb, "%s: %d (%s), sequence: %d, resource id: %d, major code: %d (%s), minor code: %d",
message,
int(error->error_code), xcb_errors[clamped_error_code],
int(error->sequence), int(error->resource_id),
Expand Down Expand Up @@ -2103,20 +2104,34 @@ void QXcbConnection::initializeShm()
{
const xcb_query_extension_reply_t *reply = xcb_get_extension_data(m_connection, &xcb_shm_id);
if (!reply || !reply->present) {
qWarning("QXcbConnection: MIT-SHM extension is not present on the X server.");
qCDebug(lcQpaXcb, "MIT-SHM extension is not present on the X server");
return;
}

has_shm = true;

auto shm_query = Q_XCB_REPLY(xcb_shm_query_version, m_connection);
if (!shm_query) {
qWarning("QXcbConnection: Failed to request MIT-SHM version");
return;
if (shm_query) {
has_shm_fd = (shm_query->major_version == 1 && shm_query->minor_version >= 2) ||
shm_query->major_version > 1;
} else {
qCWarning(lcQpaXcb, "QXcbConnection: Failed to request MIT-SHM version");
}

has_shm_fd = (shm_query->major_version == 1 && shm_query->minor_version >= 2) ||
shm_query->major_version > 1;
qCDebug(lcQpaXcb) << "Has MIT-SHM :" << has_shm;
qCDebug(lcQpaXcb) << "Has MIT-SHM FD :" << has_shm_fd;

// Temporary disable warnings (unless running in debug mode).
auto logging = const_cast<QLoggingCategory*>(&lcQpaXcb());
bool wasEnabled = logging->isEnabled(QtMsgType::QtWarningMsg);
if (!logging->isEnabled(QtMsgType::QtDebugMsg))
logging->setEnabled(QtMsgType::QtWarningMsg, false);
if (!QXcbBackingStore::createSystemVShmSegment(this)) {
qCDebug(lcQpaXcb, "failed to create System V shared memory segment (remote "
"X11 connection?), disabling SHM");
has_shm = has_shm_fd = false;
}
if (wasEnabled)
logging->setEnabled(QtMsgType::QtWarningMsg, true);
}

void QXcbConnection::initializeXFixes()
Expand Down

0 comments on commit 67227ae

Please sign in to comment.