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

Introduce and use EscapeBusAddressValue #302

Merged
merged 1 commit into from
Feb 13, 2022

Conversation

kolyshkin
Copy link
Contributor

D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that).

More to say, if dbus sees a character that should have been escaped, it
returns an error [3].

Add EscapeBusAddressValue function (with some tests and a benchmark),
and use it from tryDiscoverDbusSessionBusAddress().

The function is exported as it might be of use to other packages (in
particular, runc [4]).

NOTE that the function does not escape '*' symbol, because de-facto it
is the list of optionally-escaped characters since 2007, but that is not
yet documented in D-Bus spec (see [5] for more details).

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330
[4] opencontainers/runc#3356
[5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 1, 2022
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that).

More to say, if dbus sees a character that should have been escaped, it
returns an error [3].

Add escapeBusAddressValue function (with some tests and a benchmark),
and use it from tryDiscoverDbusSessionBusAddress().

NOTE that the function does not escape '*' symbol, because de-facto it
is the list of optionally-escaped characters since 2007, but that is not
yet documented in D-Bus spec (see [4] for more details).

TODO: switch to dbus.EscapeBusAddressValue if/when
godbus/dbus#302 is merged.

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330
[5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin force-pushed the escape-bus-address branch 2 times, most recently from 47b6dc1 to 1c520fd Compare February 3, 2022 03:57
Copy link
Member

@guelfey guelfey left a comment

Choose a reason for hiding this comment

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

Makes sense. Shouldn't we then consequently unescape the address in e.g. Dial?

@kolyshkin
Copy link
Contributor Author

Well, it seems that getKey needs unescaping, as spec says only the key values should be escaped.

D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that).

More to say, if dbus sees a character that should have been escaped, it
returns an error [3].

Add EscapeBusAddressValue function (with some tests and a benchmark),
and use it from tryDiscoverDbusSessionBusAddress().

Add UnescapeBusAddressValue, and use it from getKey().

The functions are exported as they might be of use to other packages
(in particular, runc [4]).

NOTE that the Escape... function does not escape '*' symbol, because
de-facto it is the list of optionally-escaped characters since 2007, but
that is not yet documented in D-Bus spec (see [5] for more details).

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330
[4] opencontainers/runc#3356
[5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Added UnescapeBusAddressValue, and use it from getKey (which actually gets the value, not key).

@guelfey
Copy link
Member

guelfey commented Feb 13, 2022

Thanks!

@guelfey guelfey merged commit 041560b into godbus:master Feb 13, 2022
@kolyshkin
Copy link
Contributor Author

@guelfey is it possible to have a release with this feature included?

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 16, 2022
This is mostly to include godbus/dbus#302.

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 16, 2022
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that).

More to say, if dbus sees a character that should have been escaped, it
returns an error [3].

Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue
function, recently added to godbus [4].

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330
[4] godbus/dbus#302

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 16, 2022
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that, as well as recent godbus [3]).

More to say, it is important to perform such escaping, since if dbus
sees a character that should have been escaped but it's not, it returns
an error [4].

Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue
function, recently added to godbus [3].

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] godbus/dbus#302
[4] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330

Signed-off-by: Kir Kolyshkin <[email protected]>
@guelfey
Copy link
Member

guelfey commented Feb 27, 2022

Done in https://github.com/godbus/dbus/releases/tag/v5.1.0 - bumped the minor version to reflect the added APIs

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Mar 1, 2022
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that, as well as recent godbus [3]).

More to say, it is important to perform such escaping, since if dbus
sees a character that should have been escaped but it's not, it returns
an error [4].

Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue
function, recently added to godbus [3].

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] godbus/dbus#302
[4] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330

Signed-off-by: Kir Kolyshkin <[email protected]>
dims pushed a commit to dims/libcontainer that referenced this pull request Oct 19, 2024
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that, as well as recent godbus [3]).

More to say, it is important to perform such escaping, since if dbus
sees a character that should have been escaped but it's not, it returns
an error [4].

Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue
function, recently added to godbus [3].

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] godbus/dbus#302
[4] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/containerd-cgroups that referenced this pull request Nov 6, 2024
D-Bus specification [1] requires that the values in server address need
to be escaped in a special way, and other clients perform the needed
escaping (e.g. systemd [2] does that, as well as recent godbus [3]).

More to say, it is important to perform such escaping, since if dbus
sees a character that should have been escaped but it's not, it returns
an error [4].

Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue
function, recently added to godbus [3].

[1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses
[2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318
[3] godbus/dbus#302
[4] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330

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

Successfully merging this pull request may close these issues.

2 participants