Skip to content

fix: prevent deadlock during Wayland client destruction #819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Dami-star wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −11
Open

fix: prevent deadlock during Wayland client destruction #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion waylib/src/server/kernel/private/wserver_p.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (C) 2023 JiDe Zhang <zhangjide@deepin.org>.
// Copyright (C) 2023-2026 JiDe Zhang <zhangjide@deepin.org>.
// SPDX-License-Identifier: Apache-2.0 OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only

#pragma once
Expand Down Expand Up @@ -42,6 +42,9 @@ class Q_DECL_HIDDEN WServerPrivate : public WObjectPrivate

GlobalFilterFunc globalFilterFunc = nullptr;
void *globalFilterFuncData = nullptr;

bool isProcessingEvents = false;
void safeFlushClients();
};

WAYLIB_SERVER_END_NAMESPACE
48 changes: 38 additions & 10 deletions waylib/src/server/kernel/wserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (C) 2023 JiDe Zhang <zhangjide@deepin.org>.
// Copyright (C) 2023-2026 JiDe Zhang <zhangjide@deepin.org>.
// SPDX-License-Identifier: Apache-2.0 OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only

#include <QObject>

Check warning on line 4 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QObject> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#define private public
#include <QCoreApplication>
#include <private/qhighdpiscaling_p.h>
Expand All @@ -23,12 +23,13 @@
#include <QEvent>
#include <QCoreApplication>
#include <QAbstractEventDispatcher>
#include <QSocketNotifier>

Check warning on line 26 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QSocketNotifier> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QMutex>

Check warning on line 27 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QMutex> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QDebug>

Check warning on line 28 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QDebug> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QScopedValueRollback>

Check warning on line 29 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QScopedValueRollback> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QProcess>

Check warning on line 30 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QProcess> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QLocalServer>

Check warning on line 31 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QLocalServer> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <QLocalSocket>

Check warning on line 32 in waylib/src/server/kernel/wserver.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <QLocalSocket> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <private/qthread_p.h>
#include <private/qguiapplication_p.h>
#include <qpa/qplatformthemefactory_p.h>
Expand Down Expand Up @@ -104,18 +105,26 @@
loop = wl_display_get_event_loop(display->handle());
int fd = wl_event_loop_get_fd(loop);

auto processWaylandEvents = [this] {
sockNot.reset(new QSocketNotifier(fd, QSocketNotifier::Read));
QObject::connect(sockNot.get(), &QSocketNotifier::activated, q, [this] {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider extracting the shared re-entrancy guard into a helper (e.g., withEventProcessingGuard) to centralize the logic and avoid duplicated checks in both lambdas.

You can factor out the re-entrancy guard to reduce duplication and make the control flow clearer without changing behavior.

Right now both lambdas do:

QObject::connect(sockNot.get(), &QSocketNotifier::activated, q, [this] {
    if (isProcessingEvents)
        return;

    QScopedValueRollback<bool> guard(isProcessingEvents, true);

    int ret = wl_event_loop_dispatch(loop, 0);
    ...
});

QObject::connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, q, [this] {
    if (isProcessingEvents)
        return;

    safeFlushClients();
});

You can keep the current semantics (activated → dispatch only, aboutToBlock → flush only) but centralize the guard logic:

// in WServerPrivate
template<typename F>
void WServerPrivate::withEventProcessingGuard(F &&fn)
{
    if (isProcessingEvents)
        return;

    QScopedValueRollback<bool> guard(isProcessingEvents, true);
    std::forward<F>(fn)();
}

Then wire the handlers as:

sockNot.reset(new QSocketNotifier(fd, QSocketNotifier::Read));
QObject::connect(sockNot.get(), &QSocketNotifier::activated, q, [this] {
    withEventProcessingGuard([this] {
        int ret = wl_event_loop_dispatch(loop, 0);
        if (ret)
            fprintf(stderr, "wl_event_loop_dispatch error: %d\n", ret);
    });
});

// Match upstream wl_display_run order: flush before dispatch.
QAbstractEventDispatcher *dispatcher = QThread::currentThread()->eventDispatcher();
QObject::connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, q, [this] {
    withEventProcessingGuard([this] {
        safeFlushClients();
    });
});

This:

  • Keeps all existing behavior (no new flush on the dispatch path; aboutToBlock still only flushes).
  • Removes duplicated isProcessingEvents checks and QScopedValueRollback boilerplate.
  • Makes it obvious that all event-related work runs under a single, shared re-entrancy guard.

if (isProcessingEvents)
return;

QScopedValueRollback<bool> guard(isProcessingEvents, true);

int ret = wl_event_loop_dispatch(loop, 0);
if (ret)
fprintf(stderr, "wl_event_loop_dispatch error: %d\n", ret);
wl_display_flush_clients(display->handle());
};

sockNot.reset(new QSocketNotifier(fd, QSocketNotifier::Read));
QObject::connect(sockNot.get(), &QSocketNotifier::activated, q, processWaylandEvents);
});

// Match upstream wl_display_run order: flush before dispatch.
QAbstractEventDispatcher *dispatcher = QThread::currentThread()->eventDispatcher();
QObject::connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, q, processWaylandEvents);
QObject::connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, q, [this] {
if (isProcessingEvents)
return;

safeFlushClients();
});

for (auto socket : std::as_const(sockets))
initSocket(socket);
Expand All @@ -127,6 +136,12 @@
{
W_Q(WServer);

// Disconnect event handlers BEFORE destroying clients to prevent
// callbacks from firing during client destruction.
sockNot.reset();
if (auto *dispatcher = QThread::currentThread()->eventDispatcher())
QObject::disconnect(dispatcher, nullptr, q, nullptr);

if (display)
wl_display_destroy_clients(*display);

Expand All @@ -137,9 +152,22 @@
(*i)->destroy(q);
delete *i;
}
}

sockNot.reset();
QThread::currentThread()->eventDispatcher()->disconnect(q);
// Replace wl_display_flush_clients: its wl_list_for_each_safe loop deadlocks
// when destroy-signal cascades make the pre-saved next node self-linked.
void WServerPrivate::safeFlushClients()
{
struct wl_list *head = wl_display_get_client_list(display->handle());
Comment on lines +159 to +161
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Guard against display being null when safeFlushClients is called after stop() teardown

Since safeFlushClients() is connected to aboutToBlock, it may still run after stop() if the signal was queued/emitted during teardown. In that case display may already be null or destroyed, so wl_display_get_client_list(display->handle()) could dereference invalid memory and crash. Please add a guard at the start of safeFlushClients() (e.g. if (!display) return;, and optionally check display->handle() as well) to handle late/queued callbacks safely during shutdown.

struct wl_list *node = head->next;
while (node != head) {
// Self-linked node: already destroyed, stop to avoid infinite loop.
if (node->next == node)
break;
struct wl_list *next = node->next;
wl_client_flush(wl_client_from_link(node));
node = next;
}
}

void WServerPrivate::initSocket(WSocket *socketServer)
Expand Down
Loading