Conversation
troglobit
force-pushed
the
next
branch
2 times, most recently
from
May 18, 2026 01:12
7983f3d to
7e771b8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Verified protocol/behavior mismatches (D-Bus header flags, runlevel signal encoding, and error mapping) can break interoperability and/or cause incorrect runtime/test behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR introduces an in-tree, brokerless D-Bus implementation (“libink”) and wires Finit (PID 1) and initctl onto it, along with an accompanying smoke-test suite and documentation updates.
Changes:
- Add libink (server+client) and a Finit-side D-Bus object tree (Manager1/Service1/Cond1) with signal emission hooks.
- Update build system/configure logic to optionally enable D-Bus support and install a system-bus policy file.
- Add D-Bus-focused smoke tests plus a small client used by the tests.
File summaries
| File | Description |
|---|---|
| test/src/Makefile.am | Build the dbus-auth-client test helper when DBUS is enabled |
| test/src/dbus-auth-client.c | New minimal client for libink smoke tests (auth/methods/signals) |
| test/src/.gitignore | Ignore dbus-auth-client test binary |
| test/setup-sysroot.sh | Copy dbus-auth-client into sysroot when present; reduce ldconfig verbosity |
| test/Makefile.am | Add dbus-*.sh tests to EXTRA_DIST and TESTS under DBUS |
| test/lib/setup.sh | Adjust sysroot freshness check for finit binary |
| test/lib/Makefile.am | Distribute dbus-setup.sh helper |
| test/lib/dbus-setup.sh | Shared D-Bus test preamble (locate client/bus socket, wait for bus) |
| test/dbus-service.sh | Smoke test for Service1 object surface + ServiceStateChanged signal |
| test/dbus-manager.sh | Smoke test for Manager1 methods/properties and authz behavior |
| test/dbus-introspect.sh | Validate introspection XML handling of compound signatures |
| test/dbus-initctl.sh | Verify initctl routes through D-Bus by observing emitted signals |
| test/dbus-cond.sh | Smoke test for Cond1 methods, policy checks, and ConditionChanged signal |
| test/dbus-bus.sh | Smoke test for org.freedesktop.DBus built-ins (Hello/AddMatch/Introspect) |
| test/dbus-auth.sh | Smoke test for SASL AUTH EXTERNAL handshake behavior |
| test/check.sh | Enable D-Bus in test configure invocation |
| src/svc.c | Register/unregister per-service D-Bus objects on svc create/delete |
| src/sm.c | Emit runlevel-changed notification over D-Bus |
| src/service.h | Export compose_cmdline() and new service_reload() API |
| src/service.c | Refactor reload into public service_reload() and internal apply helper; emit state-change notifications |
| src/private.h | Add internal dbus_* prototypes guarded by HAVE_DBUS |
| src/Makefile.am | Build/link dbus.c and libink into finit/initctl under DBUS |
| src/finit.h | Add FINIT_BUS_SOCKET path constant |
| src/finit.c | Initialize D-Bus listener during startup when enabled |
| src/dbus.c | New: Finit↔libink glue, vtables, signal emission, optional system-bus attach |
| src/cond-w.c | Emit ConditionChanged signal when conditions are set/cleared (guarded) |
| src/api.c | Route legacy API reload through service_reload() |
| mkdocs.yml | Add D-Bus Integration page to docs nav |
| Makefile.am | Add libink + dbus-1 subdirs under DBUS, ensure build order |
| libink/server.c | New: server socket creation/accept, peer uid capture, attach authenticated fds |
| libink/README.md | New: library overview, layout, and testing notes |
| libink/proto.h | New: message header parse/build API and constants |
| libink/proto.c | New: message header parser/builders |
| libink/path.h | New: object-path escaping API |
| libink/path.c | New: systemd-style path segment escaping |
| libink/match.c | New: AddMatch rule parsing and matching |
| libink/marshal.h | New: internal marshal API for writer/reader |
| libink/marshal.c | New: body marshalling/unmarshalling helpers |
| libink/Makefile.am | New: build libink as a convenience libtool library |
| libink/link.h | New: public libink API (server, client, marshal helpers) |
| libink/io.c | New: shared EINTR-resilient read/write helpers |
| libink/internal.h | New: internal types/constants and private APIs |
| libink/dispatch.c | New: object registry + method dispatch + reply/signal send helpers |
| libink/connection.c | New: per-connection state machine (auth→binary dispatch) |
| libink/client.c | New: synchronous client implementation for method calls and signal wait |
| libink/builtin.c | New: built-in org.freedesktop.DBus.* handlers (Hello/Introspect/Properties/AddMatch) |
| libink/auth.c | New: SASL AUTH EXTERNAL implementation for server and client |
| libink/.gitignore | Ignore build artifacts in libink |
| doc/dbus.md | New: D-Bus Integration user guide |
| doc/ChangeLog.md | Document D-Bus/libink feature and initctl routing |
| doc/build.md | Document --disable-dbus and clarify dbus plugin vs built-in support |
| dbus-1/org.finit.conf | New: system-bus policy restricting state-changing methods |
| dbus-1/Makefile.am | Install dbus policy file when DBUS is enabled |
| dbus-1/.gitignore | Ignore generated Makefile artifacts |
| configure.ac | Add --disable-dbus, HAVE_DBUS define, and DBUS automake conditional |
Review details
Suppressed comments (3)
src/dbus.c:421
- manager_signal() also treats any non-zero dispatch_action() return as NoSuchService. If kill(2) fails for a matched/running service, this incorrectly reports a missing service instead of an operational failure.
libink/proto.c:322 - Similarly, ERROR replies should not set NO_REPLY_EXPECTED; reply frames should use flags=0 to match the D-Bus spec and maximize interop.
return finalize_header(buf, cap, LINK_MSG_ERROR,
LINK_FLAG_NO_REPLY_EXPECTED,
body_len, serial, off);
libink/proto.c:349
- Signal frames should also use flags=0; NO_REPLY_EXPECTED is a METHOD_CALL flag and setting it on SIGNALs is at best meaningless and at worst trips protocol validators.
return finalize_header(buf, cap, LINK_MSG_SIGNAL,
LINK_FLAG_NO_REPLY_EXPECTED,
body_len, serial, off);
- Files reviewed: 56/56 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| @@ -330,7 +330,6 @@ export SYSROOT | |||
| top_builddir="${top_builddir:-$TEST_DIR/..}" | |||
| sysroot_finit="$SYSROOT/sbin/finit" | |||
| built_finit="$top_builddir/src/finit" | |||
Comment on lines
+904
to
+905
| snprintf(old_s, sizeof(old_s), "%d", old_level); | ||
| snprintf(new_s, sizeof(new_s), "%d", new_level); |
Comment on lines
+307
to
+316
| const char *ident; | ||
|
|
||
| if (link_call_read_string(call, &ident) < 0) | ||
| return link_call_reply_error(call, | ||
| "org.freedesktop.DBus.Error.InvalidArgs", | ||
| "expected (s)"); | ||
| if (dispatch_action(ident, action, NULL) != 0) | ||
| return link_call_reply_error(call, | ||
| "org.finit.Error.NoSuchService", ident); | ||
|
|
Comment on lines
+293
to
+295
| return finalize_header(buf, cap, LINK_MSG_METHOD_RETURN, | ||
| LINK_FLAG_NO_REPLY_EXPECTED, | ||
| body_len, serial, off); |
troglobit
force-pushed
the
next
branch
3 times, most recently
from
August 13, 2026 07:02
a20c97b to
240ada0
Compare
Finit had no way to answer the question every service manager gets asked: what is running, and change it. D-Bus is how the rest of userspace asks, but linking libdbus, sd-bus or GIO into PID 1 buys a dependency, an allocator and a main loop we do not control. So libink: the wire format, an object tree, and a bus of Finit's own at /run/finit/bus, gated like INIT_SOCKET. It speaks the standard org.freedesktop.DBus, .Peer, .Introspectable interfaces, and Finit's own Manager1, Service1 and Cond1 on top. Methods that change something are marked privileged and answered only for a caller the kernel vouched for, via SO_PEERCRED. Server and client both, since initctl is the first thing that needs to talk to it, and its Start/Stop/Restart/Reload now go over the bus rather than the legacy socket. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The bus can already answer questions and change services, so give initctl the two things it still did another way: watching signals as they happen, and getting or setting user conditions. The dbus tests move with it, split by area rather than one file that grew every time the library did. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Runlevel and version are state, not actions, so they belong behind org.freedesktop.DBus.Properties rather than another method each. Finit also claims org.finit on the system bus when it finds one, so ordinary D-Bus clients can reach it without knowing about /run/finit/bus. Opportunistic on purpose: no dbus-daemon is a normal state for the systems Finit runs on, not an error to report. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The summary table, the per-service detail, JSON and the quiet and ident forms all read state Finit already publishes, so they read it from the bus like everything else rather than through a second path that has to be kept in step. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
libink was written against the only bus it had, its own, where the peer on the other end is the client. A broker is not: it routes for senders it names itself, expects a DESTINATION on anything addressed through it, and answers on its own schedule rather than next. Runlevels go on the wire as S and N rather than the digits Finit keeps internally, since that is what a caller outside Finit means by one. The library stays a convenience library, linked into finit and initctl and installed nowhere: the ABI promise waits until libink is its own project. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The attach is best effort and its diagnostics were tuned for the case where no broker exists, so a broker that answers but refuses us was reported as a bare rc=1 at debug level. Chasing that meant reading the header builder to find out what the number meant. Failures now quote the error name the broker sent, and the one for a name we could not claim says which of the three ways it went wrong. Repeats stay quiet. The probe runs on every service and condition change, and before syslog is up each line is an open, write and close on /dev/kmsg, so a broker that keeps refusing would otherwise flood the console during boot. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The last assertion sampled bar's state one step after asking Finit to stop foo, but bar passes through stopped on its way to waiting, so the test failed roughly one run in eight. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
libink was written peer to peer, where one connection is one client and one principal. Attaching to a message bus breaks both halves of that, and two things followed from it. Signals never reached the system bus. Fan-out is gated on the peer having sent AddMatch, but a broker subscribes for its own clients and never sends us one, so every ServiceStateChanged was dropped on the floor. A connection attached with LINK_ATTACH_BROKER gets them all. Hello, AddMatch and RemoveMatch write per-connection state. Shared by every caller behind a broker, that lets one sender exhaust the match cap or drop another's rule, so we leave all three to the bus, whose job they are. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The D-Bus socket was bound world read/write, on the reasoning that SO_PEERCRED authorizes each method anyway. That leaves the read-only surface open to every local user, and it quietly ignores --with-group: a system that restricts initctl to the wheel group still handed the same service state to anyone who asked over the bus. Bind it 0660 and chown it to the configured group, the same gate the fallback socket has always had. libink takes the mode as an argument rather than assuming one, since who may connect is the embedder's policy, not the library's. The mode is applied at bind(), so there is no window where the socket is more permissive than intended. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
On the local bus SO_PEERCRED says who is calling and the kernel is the one saying it. Behind a broker one connection carries every caller, so that credential describes dbus-daemon and nothing else, and every privileged method was refused there, root included. Ask the bus driver instead. libink parks the call and hands us the sender; we ask GetConnectionUnixUser and answer when the reply lands, through the same event loop as everything else. Nothing blocks: blocking in PID 1 is why libuEv exists. That needs calls libink can make on a connection it already has, so it gained those too. Answers are cached, since a bus never reuses a unique name while it runs. Not across a restart though: a new dbus-daemon numbers from scratch and :1.7 becomes somebody else, so the cache goes when the broker does. A sender name too long to key on is refused rather than truncated, two callers sharing a truncated key would share an identity. Privilege is no longer uid 0 alone. The socket is already owned by the --with-group group, so refusing its members every method that changes anything left a wheel user able to open the bus and unable to reboot. Both gates now say the same thing. Group membership needs NSS, which the C library loads with dlopen(), so the lookup is compiled out where Finit is built to link statically. That leaves such a build root-only, which is worth saying out loud rather than leaving to be discovered. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The legacy socket logs a line per command under initctl debug; the bus logged nothing, so the transport that now carries most of initctl was the one you could not watch. libink gets a logger hook rather than a dependency on Finit's: it passes the emitting function and a formatted message, and dbus.c hands both to logit() so the two sources read alike. Trace points cover the connection lifecycle, every inbound call, and why a call was refused. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
A peer can be dropped from inside its own read loop: a handler emits a signal, the write to that very peer fails, and the drop lands while link_connection_process() still holds the connection and will touch its rx buffer on the way out. Freeing there pulls the ground out from under it. Unlink the peer and let the event loop free it once the stack has unwound. The work has to be scheduled with a non-zero delay. A uev timer armed with zero is a disarmed timer, so the queue would never run and the connections would leak instead. Losing a peer is also not a warning. It is what shutdown looks like from here, and every reboot said so on the console. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Finit speaks D-Bus itself now and claims org.finit on the system bus when it finds one, but nothing in a default build ever brings that bus up. The plugin that does was opt-in, so the built-in support sat idle unless the integrator knew to ask for both halves. Defaulting it on is only reasonable if the result stays the admin's to change, and a service registered from C through conf_save_service() is not: it lands in the run path where it cannot be overridden or emptied out. So the daemon moves to 20-dbus.conf and its directories to tmpfiles.d/dbus.conf, the same way hotplug and every other daemon we ship them for. The plugin keeps only what has to look at the running system, the stale pidfile and the machine UUID. Those directories are no longer chowned to messagebus. tmpfiles.d skips a line whose user does not exist rather than falling back, so the plugin's messagebus/dbus/root ladder has no equivalent there, and dbus-daemon binds its socket before dropping privileges anyway. The plugin already bows out where there is no dbus-daemon installed, so systems that never wanted a bus are unaffected, and --disable-dbus-plugin is there for those that have one and would still rather init left it alone. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
The other dbus-*.sh tests drive libink's own client, so the wire format was only ever checked against the implementation that wrote it, and the broker path had no coverage at all. Every bug found in it so far was found by hand on a target. Let the dbus plugin bring up a real dbus-daemon, wait for Finit to claim org.finit, then talk to Finit with dbus-send, which shares no code with us. The privileged call is the interesting one: it can only be answered by parking the call and asking the broker who sent it. The bus reads the policy Finit installs, so a malformed org.finit.conf fails here rather than on a target. Tests no longer build --with-libsystemd. Our replacement carries the real soname but only the sd_notify() symbols, so in the test root it shadowed the libsystemd the host's libdbus-1 wants and dbus-daemon died on a missing sd_is_socket. Nothing under test needs the shared library: serv is the only consumer and it compiles sd-daemon.c straight in, which it now does regardless of the flag so notify.sh keeps testing notify:systemd either way. Staged from the host by lib/sysroot.mk like any other binary, and skipped when the host has neither program. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
It still described treating every system-bus caller as unprivileged as the state of things, which stopped being true when Finit learned to ask the broker who sent a call. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Parked calls and outbound calls awaiting a reply only ever happen on a connection talking to a broker, but the parked array sat on the server and the pending array on every connection. A server with no broker carried 4 KiB of slots it could never fill, and both were reachable from code paths that have no business in them. Move both behind one struct, allocated on the first park or call and freed with the connection. link_server_t goes from 4400 to 168 bytes; link_connection_t barely moves, its buffers dominate, but an ordinary peer no longer carries reply-tracking it never uses. Tokens are now per bus rather than per server, so link_uid_resolved() takes the connection the answer is about. Every resolver already has it: it is the first argument to both the resolver and the reply callback. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
A call is parked until the resolver says who sent it, and an outbound call sits in a pending slot until its reply lands. Neither had a way to give up. A broker that answers GetConnectionUnixUser slowly, or not at all, leaves the caller waiting forever and keeps the slot; four of those and every later privileged call is refused with LimitsExceeded until Finit restarts. libink cannot time itself out, it has no event loop, so the deadline is the embedder's to keep. One sweep per connection covers both, and the ordering between them stays in the library rather than in each embedder: calls first, because one timing out usually resolves the park it was made for, and AccessDenied tells that caller more than a bare timeout. The sweep is armed when a resolve is deferred and stops rearming as soon as nothing is outstanding, so a system that never meets a broker never wakes up for it. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Only the file header said we refuse 'B' messages, and nobody reads a header comment when they are looking at why a parse failed. Put it at the check, with an XXX so it turns up in a grep for known gaps. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
"Set not yet implemented" reads as a promise. Finit exposes no writable property and has no use for one: everything a caller might want to change is a Manager1 method, where the authorization lives. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
A rule carrying sender, destination, or eavesdrop was refused whole, and a peer whose AddMatch fails gets no signals at all. That is a poor trade for keys clients attach as a matter of course: better a filter wider than asked for than a subscription that never happened. They are accepted and ignored rather than honoured. Widening costs nothing here since Finit is the only sender on this bus, and what it emits through the match table is state any peer that got this far may already read. argN and argNpath still take the whole rule down. Honouring them means parsing message bodies, and nothing asks for them yet. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
__msg_parse() turns bytes off a socket into pointers, before anything has vouched for the peer, and it is the only place in libink that does. It had no test of its own beyond whatever the other tests happened to send it, all of it well-formed. The target checks the parser's contract, not merely that it survived. A header field must point into the header field array, and terminate inside it, and the parse must never claim more bytes than it was handed. Crash-only would pass a parser that walked into the body and returned fields from there, since those bytes were handed over too. The expected bounds are derived from the raw header rather than from the parser, so the two have to agree independently. Every input is copied into an allocation sized to it first. Reading past the end of a roomy buffer stays inside the allocation and the sanitizer never sees it; against an exact one the same read is a fault, which is where the sharpest findings come from. Under libFuzzer it is an ordinary fuzz target and named files replay, which is how a find gets reproduced. With no arguments it runs a fixed sweep -- every truncation, every single-byte corruption, every value of the length that decides where the header ends, and seeded garbage -- so the suite covers the same contract on every build, without clang or a corpus in the tree. It takes 40 ms. CI fuzzes it properly on every pull request, keeps the crashers, and carries the corpus between runs so it reaches deeper over time than any single run can. Note that clang links the fuzzer runtime against the newest GCC tree it finds, so the libstdc++ headers have to match that one and not the default compiler, which is worth saying since installing the obvious package leaves you exactly where you started. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 78 out of 79 changed files in this pull request and generated no new comments.
Suppressed comments (9)
test/lib/setup.sh:332
- When finit is linked with libtool libraries, $top_builddir/src/finit is typically a libtool wrapper script while the real binary is in src/.libs/finit. Comparing/copying the wrapper makes the stale-sysroot check fail even when the sysroot is up to date.
test/dbus-initctl.sh:116 - Restore path should match the moved socket path (see earlier mv). Using /run/finit/socket will fail on glibc-based sysroots without a /var/run -> /run symlink.
libink/proto.c:298 - NO_REPLY_EXPECTED is a MethodCall flag; setting it on METHOD_RETURN frames is non-standard and can confuse strict D-Bus tooling. Replies should use flags=0 here.
libink/proto.c:352 - Signals do not expect replies by definition; setting LINK_FLAG_NO_REPLY_EXPECTED is redundant and non-standard. Using flags=0 improves interoperability with strict parsers.
test/dbus-auth.sh:25 - This test hard-codes INIT_SOCKET as /run/finit/socket, but the build-time path uses _PATH_VARRUN ("/var/run/" on glibc). In the test sysroot, /var/run is not a symlink to /run, so stat() will fail or check the wrong socket.
test/lib/dbus-setup.sh:14 - BUS is set to /run/finit/bus, but FINIT_BUS_SOCKET is built from _PATH_VARRUN ("/var/run/" on glibc). The test sysroot skeleton does not create /var/run -> /run, so the socket will appear under /var/run/finit/bus and these tests will wait forever.
test/src/.gitignore:6 - test/src/Makefile.am builds an additional noinst_PROGRAMS binary (fuzz-msg-parse) under test/src/, but it is not ignored. This will leave an untracked file after builds/tests.
libink/proto.c:325 - Same issue as METHOD_RETURN: ERROR replies should not set LINK_FLAG_NO_REPLY_EXPECTED. Use flags=0 for reply message types.
This issue also appears on line 350 of the same file.
test/dbus-initctl.sh:101
- The legacy socket path is hard-coded as /run/finit/socket, but INIT_SOCKET is built from _PATH_VARRUN ("/var/run/" on glibc). In the test sysroot, /var/run is not a symlink to /run, so hiding /run/finit/socket may not actually hide the socket initctl uses.
This issue also appears on line 113 of the same file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial work on what could become Finit 5.0 ...