Plug memory leak: add missing dbus_message_unref

[kkzhsh]

This patch fixes memory leaks where DBusMessage objects created with
dbus_message_new_signal() were not unreferenced after dbus_connection_send().

According to libdbus documentation, dbus_connection_send() does not take
ownership of the message, so the caller must still call dbus_message_unref().

Fixed in:

• sbus_server_name_owner_changed()
• sbus_server_name_acquired()
• sbus_server_name_lost()
• sbus_sync_message_send()

[gemini-code-assist]

Code Review

This pull request addresses memory leaks by adding missing dbus_message_unref calls in sbus_server.c and sbus_sync.c. While the additions in the server component are straightforward, the feedback highlights a significant issue in sbus_sync.c where the unreference is only applied conditionally. This creates inconsistent ownership semantics and leaves a memory leak in the branch where a reply is expected. It is recommended to move the cleanup to a common exit point to ensure the message is always released regardless of the reply status.

[gemini-code-assist]

The addition of dbus_message_unref(msg) only in the _reply == NULL case introduces inconsistent ownership for the msg parameter. Callers will now have to know whether they passed a reply pointer or not to decide if they should unreference the message themselves, which is error-prone. Furthermore, the message is still leaked in the _reply != NULL branch (lines 140+), as dbus_connection_send_with_reply_and_block does not take ownership of the message. It is recommended to handle the unreference consistently for both branches, ideally by moving it to the end of the function (e.g., at the done label) to ensure the message is always cleaned up regardless of whether a reply was requested.

[kkzhsh]

This PR has been updated, review again

[alexey-tikhonov]

/gemini see update

[gemini-code-assist]

Thank you for the update. I have reviewed the changes in src/sbus/sync/sbus_sync.c. While the current fix addresses the leak in the _reply == NULL branch, it still leaves the msg object unreferenced in the _reply != NULL branch (lines 140+), as dbus_connection_send_with_reply_and_block does not take ownership of the message. To ensure consistent memory management and avoid potential leaks, it is best practice to unreference the message at a single exit point (e.g., a done label) regardless of the branch taken. Please consider refactoring to handle the dbus_message_unref(msg) call in one place.

[kkzhsh]

/gemini
src/sbus/sync/sbus_sync.c(line:140) is function sbus_sync_message_send, There are two call paths for sbus_sync_message_send
1） sbus_sync_call_signal->sbus_sync_emit_signal->sbus_sync_message_send, and sbus_sync_call_signal has been added with dbus_message_unref after call sbus_sync_emit_signal
2) sbus_sync_call_method->sbus_sync_message_send, in funciton sbus_sync_call_method, the message is bounded with talloc context, and it is managerd with talloc

[gemini-code-assist]

Thank you for the clarification regarding the ownership and lifecycle of the msg object in sbus_sync_message_send. Given that sbus_sync_call_signal handles the unreference and sbus_sync_call_method relies on talloc for memory management, the current implementation is correct in those contexts. I appreciate you taking the time to explain the call paths.

[sssd-bot]

Review done using Claude Code / claude-opus-4-6

Functional Issues

Double-unref in sbus_call_signal_send — will corrupt heap or crash

The added dbus_message_unref(msg) at src/sbus/request/sbus_request_call.c:160 is incorrect and introduces a double-unref.

sbus_emit_signal() (src/sbus/connection/sbus_send.c:241-244) is a thin wrapper around sbus_reply() (src/sbus/connection/sbus_send.c:208-218), which already calls dbus_message_unref(reply) on both the normal path (line 217) and the disconnecting path (line 212):

void sbus_reply(struct sbus_connection *conn, DBusMessage *reply)
{
    if (conn->disconnecting) {
        dbus_message_unref(reply);  /* unrefs here */
        return;
    }

    dbus_connection_send(conn->connection, reply, NULL);
    dbus_message_unref(reply);  /* and here */
}

So sbus_emit_signal() takes ownership of the message. After it returns, the caller no longer holds a reference. The PR's added dbus_message_unref(msg) decrements the refcount past the caller's ownership — on the normal path this releases the D-Bus connection's internal reference prematurely (use-after-free); on the disconnecting path (where dbus_connection_send is skipped) it unrefs an already-freed message.

This line must be removed. The pre-PR code was correct for this call site — there was no leak here.

Note the asymmetry: sbus_emit_signal() (async) takes ownership via sbus_reply(), but sbus_sync_emit_signal() (sync) does not take ownership — it calls sbus_sync_message_send() with _reply == NULL, which sends via dbus_connection_send + flush without unreffing. The fix in src/sbus/sync/sbus_sync_call.c:101 is therefore correct.

Summary of each fix site:

Location	Correct?	Reason
src/sbus/server/sbus_server.c:509 (sbus_server_name_owner_changed)	Yes	sbus_server_matchmaker() does not take ownership
src/sbus/server/sbus_server.c:529 (sbus_server_name_acquired)	Yes	dbus_connection_send() does not take ownership
src/sbus/server/sbus_server.c:556 (sbus_server_name_lost)	Yes	dbus_connection_send() does not take ownership
src/sbus/sync/sbus_sync_call.c:101 (sbus_sync_call_signal)	Yes	sbus_sync_emit_signal() does not take ownership
src/sbus/request/sbus_request_call.c:160 (sbus_call_signal_send)	No	sbus_emit_signal() already takes ownership via sbus_reply()

Nits & Non-Functional Issues

• The commit message lacks the component prefix (should be sbus: Plug memory leak... per SSSD conventions).
• Since this fixes user-visible memory leaks (gradual memory growth of long-running SSSD daemons), the commit message should include a :relnote: tag.
• Pre-existing issue worth noting: sbus_emit_signal() taking ownership while sbus_sync_emit_signal() does not is an API inconsistency that makes reasoning about message lifetime error-prone — as demonstrated by this PR.

Review of Existing Review Comments

The Gemini bot comments on src/sbus/sync/sbus_sync.c reference a previous revision of the PR that apparently modified sbus_sync_message_send(). That file is no longer changed in the current revision. Gemini's observation about a pre-existing leak of the reply object in sbus_sync_message_send() on error paths (e.g., when sbus_reply_check() fails at src/sbus/sync/sbus_sync.c:148-151) is valid but is a separate pre-existing issue outside the scope of this PR.

[kkzhsh]

@sssd-bot review again

[alexey-tikhonov]

Double-unref in sbus_call_signal_send — will corrupt heap or crash

This was fixed.