NULL pointer dereference in atomic DRM backend

[ziyao233]

Hyprland 0.45.0 together with aquamarine 0.4.4, hyprutils 0.2.4, segfaults on start. The binary is built by clang 19.1.3 and linked against libc++. Hyprland was started with

HYPRLAND_NO_CRASHREPORTER=1 HYPRLAND_TRACE=1 AQ_TRACE=1 Hyprland

and corresponding log is attached1. Some utf8-unclean lines are stripped and they're unrelated to this problem.

Examining the core, it's found that an invalid pointer is dereferenced in Aquamarine::CDRMAtomicRequest::commit()

$ lldb Hyprland --core core
(lldb) target create "Hyprland" --core "core"
Core file '/home/ziyao/packaging/change_aquamarine/core' (x86_64) was loaded.
(lldb) bt
* thread #1, name = 'Hyprland', stop reason = signal SIGSEGV: address not mapped to object
  * frame #0: 0x00007f217f8e426a libaquamarine.so.3`Aquamarine::CDRMAtomicRequest::commit(unsigned int) + 74
    frame #1: 0x00007f217f8e4f34 libaquamarine.so.3`Aquamarine::CDRMAtomicImpl::reset() + 596
    frame #2: 0x00007f217f8c4dd8 libaquamarine.so.3`Aquamarine::CDRMBackend::start() + 24
    frame #3: 0x00007f217f8797b9 libaquamarine.so.3`Aquamarine::CBackend::start() + 217
    frame #4: 0x000055896fce6e1b Hyprland`CCompositor::initServer(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, int) + 1407
    frame #5: 0x000055896fddfeda Hyprland`main + 1707
(lldb)

More digging shows it is this->conn dereferenced in src/backend/drm/impl/Atomic.cpp2 that contains a NULL address, leading to a segmentation fault,

    if (auto ret = drmModeAtomicCommit(backend->gpu->fd, req, flagssss, &conn->pendingPageFlip); ret) {
        backend->log((flagssss & DRM_MODE_ATOMIC_TEST_ONLY) ? AQ_LOG_DEBUG : AQ_LOG_ERROR,
                     std::format("atomic drm request: failed to commit: {}, flags: {}", strerror(-ret), flagsToStr(flagssss)));
        return false;
    }

It turns out that Aquamarine::CDRMAtomicImpl::reset() doesn't call Aquamarine::CDRMAtomicRequest::commit() in a correct code sequence3,

bool Aquamarine::CDRMAtomicImpl::reset() {
    CDRMAtomicRequest request(backend);


    for (auto const& crtc : backend->crtcs) {
        request.add(crtc->id, crtc->props.mode_id, 0);
        request.add(crtc->id, crtc->props.active, 0);
    }


    for (auto const& conn : backend->connectors) {
        request.add(conn->id, conn->props.crtc_id, 0);
    }


    for (auto const& plane : backend->planes) {
        request.planeProps(plane, nullptr, 0, {});
    }


    return request.commit(DRM_MODE_ATOMIC_ALLOW_MODESET);
}

The member conn of the newly created CDRMAtomicRequest request isn't initialized before commit() is called, which I think should be done through CDRMAtomicRequest::addConnector(). And when problematic CDRMAtomicImpl::reset() is invoked by CDRMBackend::start() during startup4, Hyprland crashes.

I'm not sure why this doesn't cause a problem before, as the piece of code exists since June....

A simple fix (or more like a workaround), is to check whether conn is valid before dereference it.

diff --git a/src/backend/drm/impl/Atomic.cpp b/src/backend/drm/impl/Atomic.cpp
index b031076..254f4f5 100644
--- a/src/backend/drm/impl/Atomic.cpp
+++ b/src/backend/drm/impl/Atomic.cpp
@@ -162,6 +162,10 @@ bool Aquamarine::CDRMAtomicRequest::commit(uint32_t flagssss) {
         return false;
     }
 
+    if (!conn.get()) {
+        return false;
+    }
+
     if (auto ret = drmModeAtomicCommit(backend->gpu->fd, req, flagssss, &conn->pendingPageFlip); ret) {
         backend->log((flagssss & DRM_MODE_ATOMIC_TEST_ONLY) ? AQ_LOG_DEBUG : AQ_LOG_ERROR,
                      std::format("atomic drm request: failed to commit: {}, flags: {}", strerror(-ret), flagsToStr(flagssss)));
@@ -337,6 +341,7 @@ bool Aquamarine::CDRMAtomicImpl::commit(Hyprutils::Memory::CSharedPointer<SDRMCo
 }
 
 bool Aquamarine::CDRMAtomicImpl::reset() {
+
     CDRMAtomicRequest request(backend);
 
     for (auto const& crtc : backend->crtcs) {
@@ -365,4 +370,4 @@ bool Aquamarine::CDRMAtomicImpl::moveCursor(SP<SDRMConnector> connector, bool sk
     }
 
     return true;
-}
\ No newline at end of file
+}

which has been applied on eweOS5 and fixed the crash. It's easy to find that CDRMAtomicImpl::reset() is used only few times in the codebase, so there may be a cleaner solution.

[vaxerski]

MRs welcome