Skip to content
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.

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

Wrapper for DBusMessageIter doesn't reference the DBusMessage used to initialize it #20

Open
ziyao233 opened this issue Jan 20, 2025 · 2 comments · May be fixed by #21
Open

Wrapper for DBusMessageIter doesn't reference the DBusMessage used to initialize it #20

ziyao233 opened this issue Jan 20, 2025 · 2 comments · May be fixed by #21
Labels

Comments

@ziyao233
Copy link

After a DBusMessageIter is initialized with a DBusMessage through dbus_message_iter_init(), the iterator actually references the message: usage of the iterator requires the message to stay valid.

ldbus doesn't claim the reference, so it's possible that a message gets garbage collected while some iterators still reference it, thus usage of such a iterator triggers a segfault, for example,

local ldbus		= require "ldbus";
collectgarbage("stop");
local msg = ldbus.message.new_method_call("org.freedesktop.DBus",
			"/org/freedesktop/DBus",
			"org.freedesktop.DBus.Debug.Stats",
			"GetAllMatchRules");
local iter = ldbus.bus.get("session"):
		send_with_reply_and_block(msg):
		iter_init();
collectgarbage(); -- Commenting this line prevents the segfault
print(iter:get_arg_type());

segfaults during iter:get_arg_type() (ldbus master with Lua 5.4), since the message returned by send_with_reply_and_block() has been collected, which makes iter a dangdling reference.

Suggest fix: when creating/cloning new iterators, reference the underlying dbus.message with luaL_ref() in the registry table, and unref the message during __gc.

@daurnimator
Copy link
Owner

huh. Seems weird to me that the DBusMessageIter doesn't itself have a ref to the DBusMessage.

Suggest fix: when creating/cloning new iterators, reference the underlying dbus.message with luaL_ref() in the registry table, and unref the message during __gc.

There should be no need for that level of indirection:
Option 1. increase the DBusMessage ref when we create a DBusMessageIter; and unref on __gc; however I can't seem to see a way to get the DBusMessage from the DBusMessageIter.
Option 2. keep the DBusMessage in the ldbus_DBusMessageIter userdata's uservalue.

@ziyao233
Copy link
Author

huh. Seems weird to me that the DBusMessageIter doesn't itself have a ref to the DBusMessage.

Same, since there's already some type of reference counting existing for DBusMessage. But the RC cannot be done with current API as you're allowed to memcpy an iterator. I think it's a design mistake.

Suggest fix: when creating/cloning new iterators, reference the underlying dbus.message with luaL_ref() in the registry table, and unref the message during __gc.

There should be no need for that level of indirection:
Option 1. increase the DBusMessage ref when we create a DBusMessageIter; and unref on __gc; however I can't seem to see a way to get the DBusMessage from the DBusMessageIter.
Option 2. keep the DBusMessage in the ldbus_DBusMessageIter userdata's uservalue.

Maybe 2 is the only option.

ziyao233 added a commit to ziyao233/ldbus that referenced this issue Feb 1, 2025
Weirdly, an DBusMessageIter doesn't increase the refcount of the
referred DBusMessage, thus we need to maintain the reference manually to
avoid underlying DBusMessage gets garbagecollected before the
DBusMessageIter, which results in a dangling reference.

Let's introduce a new type, lDBusMessageIter, which contains both the
iterator and the underlying DBusMessage, and refer/unref the message
during creation, initialization and garbage collection of the iterator.

Closes: daurnimator#20
Signed-off-by: Yao Zi <[email protected]>
ziyao233 added a commit to ziyao233/ldbus that referenced this issue Feb 1, 2025
Weirdly, an DBusMessageIter doesn't increase the refcount of the
referred DBusMessage, thus we need to maintain the reference manually to
prevent underlying DBusMessage from garbage collection before the
DBusMessageIter is collected, which results in a dangling reference.

Let's introduce a new type, lDBusMessageIter, which contains both the
iterator and the underlying DBusMessage, and refer/unref the message
during creation, initialization and garbage collection of the iterator.

Closes: daurnimator#20
Signed-off-by: Yao Zi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants