Bluetooth-Devices / dbus-fast

A faster version of dbus-next
MIT License
35 stars 8 forks source link

Introspection returns bogus child paths #279

Closed mriemensberger closed 4 months ago

mriemensberger commented 4 months ago

Describe the bug

I see the following path hierarchy when showing the object tree with busctl tree com.example.Service

└─/com
  └─/com/example
    └─/com/example/service
      ├─/com/example/service/item
      │ ├─/com/example/service/item/0
      │ ├─/com/example/service/item/1
      │ │ ├─/com/example/service/item/1/0
      │ │ └─/com/example/service/item/1/1
      │ ├─/com/example/service/item/10
      │ ├─/com/example/service/item/11
      │ └─/com/example/service/item/2
...

I would expect to see only

└─/com
  └─/com/example
    └─/com/example/service
      ├─/com/example/service/item
      │ ├─/com/example/service/item/0
      │ ├─/com/example/service/item/1
      │ ├─/com/example/service/item/10
      │ ├─/com/example/service/item/11
      │ └─/com/example/service/item/2
...

The /com/example/service/item/1/0 and /com/example/service/item/1/1 are bogus and have never been exported by the service. They also show up without any interfaces when being introspected with busctl introspect.

It looks like that the computation of the children nodes in https://github.com/Bluetooth-Devices/dbus-fast/blob/f0fc6668b9543bd81e2438fe78a54c31702a4bc5/src/dbus_fast/message_bus.py#L650 is wrong if some paths are prefixes for other paths. For example, /com/example/service/item/1 is a prefix of /com/example/service/item/10, but the latter is not a child of the former.

To Reproduce

Additional context

Suggested fix: Replace https://github.com/Bluetooth-Devices/dbus-fast/blob/f0fc6668b9543bd81e2438fe78a54c31702a4bc5/src/dbus_fast/message_bus.py#L663 with

            if not export_path.startswith(path + '/'):
bdraco commented 4 months ago

That looks like a reasonable fix. It might need an equality check as well for the exact path (but not sure on that one without a test)

mriemensberger commented 4 months ago

I ended up with this fix:

--- a/src/dbus_fast/message_bus.py  2024-05-01 00:20:01.242137424 +0000
+++ b/src/dbus_fast/message_bus.py  2024-05-01 00:20:17.490082951 +0000
@@ -665,6 +665,9 @@

             child_path = export_path.split(path, maxsplit=1)[1]
+            if path and path[-1] != "/" and child_path and child_path[0] != "/":
+                continue
+
             child_path = child_path.lstrip("/")
             child_name = child_path.split("/", maxsplit=1)[0]

             children.add(child_name)
mriemensberger commented 4 months ago

Otherwise the root path introspection (path = "/" doesn't work).

mriemensberger commented 4 months ago

But there is probably a prettier way to fix it.

bdraco commented 4 months ago

If you open a PR with a test, I can do a proper review this weekend assuming the Home Assistant release tomorrow doesn't eat up all my available time

mriemensberger commented 4 months ago

PR #280 created. Please review.

mriemensberger commented 4 months ago

@bdraco Thanks for handling this so quickly.

bdraco commented 4 months ago

Thank you for your contributions!