fix: free xcb_reply's with scoped smart pointers #4592

aliihsancengiz · 2025-12-26T20:05:23Z

Related: #4559

What's new?

Previously, some of xcb_*_reply results were never freed; this PR uses a scope-based smart pointer to free results when they get out of scope.

How to test

Checklist

Tests added and pass
Adequate documentation added
(optional) Added Screenshots or videos

AlanGriffiths

One readability tweak.

But this there are other xcb results in the code that could have the same treatment:

xcb_render_query_pict_formats_reply
xcb_poll_for_event
xcb_list_properties_reply

Either these should be addressed, or the issue should remain open

AlanGriffiths · 2026-01-05T10:31:18Z

src/platforms/x11/graphics/egl_helper.cpp

 {
    auto cookie = xcb_get_geometry(xcb_conn, win);
-    if (auto reply = xcb_get_geometry_reply(xcb_conn, cookie, nullptr))
+    if (auto const reply = mir::make_unique_cptr(xcb_get_geometry_reply(xcb_conn, cookie, nullptr)); reply)


Can be simpler:

Suggested change

if (auto const reply = mir::make_unique_cptr(xcb_get_geometry_reply(xcb_conn, cookie, nullptr)); reply)

if (auto const reply = mir::make_unique_cptr(xcb_get_geometry_reply(xcb_conn, cookie, nullptr)))

Thanks for the hint, addressed and applied for other use case of xcb_* functions you listed.

Copilot

Pull request overview

This PR addresses memory leaks in Xwayland-related code by replacing manual free() calls with scope-based smart pointers (mir::make_unique_cptr). The changes ensure that XCB reply objects are automatically freed when they go out of scope, preventing resource leaks that could occur if early returns or exceptions prevented the manual free() calls from being reached.

Key changes:

Wrapped XCB reply pointers in mir::make_unique_cptr for automatic memory management
Updated code to use .get() when accessing the underlying pointer
Added necessary #include <mir/c_memory.h> headers

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/server/frontend_xwayland/xwayland_wm.cpp	Converted multiple XCB reply allocations to use smart pointers (xfixes_reply, event polling, and property listing)
src/server/frontend_xwayland/xwayland_cursors.cpp	Wrapped formats_reply in smart pointer and added required header
src/platforms/x11/x11_resources.cpp	Applied smart pointers to atom interning and refresh rate queries
src/platforms/x11/graphics/egl_helper.cpp	Converted geometry reply to use smart pointer

Copilot · 2026-01-05T14:35:53Z

src/server/frontend_xwayland/xwayland_cursors.cpp

        }

-        free(formats_reply);
+


Empty line should be removed as it adds unnecessary whitespace after the closing brace.

AlanGriffiths

Just some include tweaking:

the first header should be the interface offered by the .cpp file
then local headers (typically those in the current directory)
then mir headers
...

AlanGriffiths · 2026-01-05T14:38:57Z

src/server/frontend_xwayland/xwayland_cursors.cpp

+#include "mir/c_memory.h"
 #include "xcb_connection.h"
 #include "xwayland_cursors.h"
 #include "xwayland_log.h"


Suggested change

#include "mir/c_memory.h"

#include "xcb_connection.h"

#include "xwayland_cursors.h"

#include "xwayland_log.h"

#include "xwayland_cursors.h"

#include "xcb_connection.h"

#include "xwayland_log.h"

#include <mir/c_memory.h>

AlanGriffiths · 2026-01-05T14:39:53Z

src/platforms/x11/graphics/egl_helper.cpp

+#include "mir/c_memory.h"



Suggested change

#include "mir/c_memory.h"

#include <mir/c_memory.h>

aliihsancengiz · 2026-01-05T17:59:03Z

Just some include tweaking:

the first header should be the interface offered by the .cpp file

then local headers (typically those in the current directory)

then mir headers
...

Thanks @AlanGriffiths , looks like my LSP is putting those out of order.

Addressed comments and rebased with main now.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

src/platforms/x11/x11_resources.cpp

Copilot · 2026-01-06T10:42:38Z

src/platforms/x11/x11_resources.cpp

        auto refresh_rate = static_cast<double>(screen_reply->rate);
        mir::log_debug("Detected %.2fHz host output refresh rate.", refresh_rate);
        return screen_reply->rate;


Missing null check before dereferencing screen_reply. If xcb_randr_get_screen_info_reply returns nullptr, line 113 will cause a null pointer dereference. Add a null check similar to the pattern used in other files.

Suggested change

auto refresh_rate = static_cast<double>(screen_reply->rate);

mir::log_debug("Detected %.2fHz host output refresh rate.", refresh_rate);

return screen_reply->rate;

if (!screen_reply)

{

mir::log_warning("Failed to get RANDR screen info; assuming 60Hz host output refresh rate.");

return 60.0;

}

auto refresh_rate = static_cast<double>(screen_reply->rate);

mir::log_debug("Detected %.2fHz host output refresh rate.", refresh_rate);

return refresh_rate;

Missing null check before dereferencing screen_reply

If there is a problem, then it is pre-existing: doesn't block this PR

AlanGriffiths

One possible simplification...

src/platforms/x11/x11_resources.cpp

AlanGriffiths · 2026-01-06T11:25:59Z

src/platforms/x11/x11_resources.cpp

        auto refresh_rate = static_cast<double>(screen_reply->rate);
        mir::log_debug("Detected %.2fHz host output refresh rate.", refresh_rate);
        return screen_reply->rate;


Missing null check before dereferencing screen_reply

If there is a problem, then it is pre-existing: doesn't block this PR

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>

aliihsancengiz requested a review from a team as a code owner December 26, 2025 20:05

aliihsancengiz force-pushed the fix/free_xcb_replies branch 2 times, most recently from 7da0273 to f8fded7 Compare December 28, 2025 18:42

AlanGriffiths requested changes Jan 5, 2026

View reviewed changes

aliihsancengiz requested a review from AlanGriffiths January 5, 2026 12:17

AlanGriffiths requested a review from Copilot January 5, 2026 14:35

Copilot AI reviewed Jan 5, 2026

View reviewed changes

AlanGriffiths requested changes Jan 5, 2026

View reviewed changes

aliihsancengiz force-pushed the fix/free_xcb_replies branch 2 times, most recently from 37cd2e2 to 741d20b Compare January 5, 2026 17:56

aliihsancengiz requested a review from AlanGriffiths January 5, 2026 17:59

AlanGriffiths requested a review from Copilot January 6, 2026 10:41

Copilot AI reviewed Jan 6, 2026

View reviewed changes

AlanGriffiths approved these changes Jan 6, 2026

View reviewed changes

aliihsancengiz and others added 3 commits January 6, 2026 13:32

fix: free xcb_reply's with scoped smart pointers

dc39a99

address review comments

d16e5cd

Apply possible suggestion

c0957e5

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>

aliihsancengiz force-pushed the fix/free_xcb_replies branch from 3acfdf9 to c0957e5 Compare January 6, 2026 12:32

AlanGriffiths added this pull request to the merge queue Jan 6, 2026

Merged via the queue into canonical:main with commit 49bb8a4 Jan 6, 2026
29 of 30 checks passed

	if (auto const reply = mir::make_unique_cptr(xcb_get_geometry_reply(xcb_conn, cookie, nullptr)); reply)
	if (auto const reply = mir::make_unique_cptr(xcb_get_geometry_reply(xcb_conn, cookie, nullptr)))

fix: free xcb_reply's with scoped smart pointers #4592

fix: free xcb_reply's with scoped smart pointers #4592

Conversation

aliihsancengiz commented Dec 26, 2025

What's new?

How to test

Checklist

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

aliihsancengiz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

aliihsancengiz commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlanGriffiths Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aliihsancengiz commented Jan 5, 2026 •

edited

Loading