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

Use USocket for UNIX_FD transport #67

Merged
merged 10 commits into from
May 20, 2021
Merged

Use USocket for UNIX_FD transport #67

merged 10 commits into from
May 20, 2021

Conversation

Niels-Be
Copy link
Contributor

Dbus allows passing of unix file descriptors using the "h" signature type.
However the spec specifies file descriptors are passed along out of band.
In case of unix domain sockets those are transported using SOL_SOCKET packet metadata.

Some DBus services e.g. Bluez rely on this functionality to hand over communication sockets to the client.

USocket is a drop in replacement for net.Socket which supports handling this metadata.

This PR will use USocket in case the DBus connection is opened over a unix domain socket and then negotiate unix fd support during the handshake.

USocket is added as an optional dependency similar to abstract-socket and if its not available the native node.js socket is used without UnixFD support.

@acrisci
Copy link
Member

acrisci commented Mar 22, 2021

Please add some tests to this.

For an example of a good testing strategy, see the implmentation on python-dbus-next here:

https://github.com/altdesktop/python-dbus-next/blob/master/test/test_fd_passing.py

@Niels-Be
Copy link
Contributor Author

Niels-Be commented Mar 23, 2021

I added tests, however they will not run in CI because the CI is using dbus-run-session which creates an abstract-socket that currently does not support unix fds.
In the test I detect that and skip the tests.

I just quickly checked but it looks like with a small change to usocket it can handel abstract sockets too. I will submit a PR there too.
Edit: jhs67/usocket#8

@Niels-Be
Copy link
Contributor Author

Niels-Be commented Mar 24, 2021

@acrisci would you mind taking a look why the CI craches?
It is working for me locally and I can not reproduce this.
I bumped usocket version, maybe this is a build cache issue?

@acrisci
Copy link
Member

acrisci commented Mar 24, 2021

The CI crash looks like some kind of double free in the socket library. It should be investigated there, but likely has something to do with how the vm is set up. I can probably fix this by running tests in a Docker container.

I would like to see some tests for the low level client as well and a bit of documentation on how to use it.

@Haochen7
Copy link

Hi @acrisci, would you be able to help with this PR when you have a chance. Really appreciate your help. Thanks

@acrisci
Copy link
Member

acrisci commented May 20, 2021

Thanks for your patience. I've been working on other projects recently so I've not been paying enough attention to this project. I tested this and it seems to work, so I will merge it as it is. However, I will make unix fd support optional with a constructor arg before I release.

@acrisci acrisci merged commit c6f794d into dbusjs:master May 20, 2021
@Haochen7
Copy link

Haochen7 commented Jun 4, 2021

Thanks @acrisci. Btw, would you happen to have a target date for the release. Thanks again!

@taoky taoky mentioned this pull request Dec 31, 2024
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.

3 participants