Conversation
troglobit
force-pushed
the
next
branch
2 times, most recently
from
May 18, 2026 01:12
7983f3d to
7e771b8
Compare
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.
A pass over the whole branch before merge, mostly in libink since that is the new code and the part exposed to the wire. Grouped here rather than scattered so the review is easy to read in one place. libink parser and dispatch: - Bound reader lengths so a 32-bit size_t can't wrap a wire length past the guard and read out of bounds. Reachable pre-auth on any bus, so it matters on the 32-bit targets Finit runs on. - Drop a peer when a reply send fails instead of limping on with a half-written frame; a built-in whose send failed used to fall through and put a second frame on the wire. initctl: - Copy a D-Bus error name out of the reply before closing the client; the reply points into memory the close frees. Both error paths now share one helper so this can't creep back. Authorization: - Take the caller's groups from the kernel (SO_PEERCRED plus SO_PEERGROUPS) rather than getpwuid()/getgrouplist(), which go through NSS and can block PID 1 on a slow LDAP or SSSD backend. The check is now a lookup against the group resolved once at init, with no NSS and no 256 KiB array on the stack. A caller reaching us through a broker carries no group set, so system-bus privileged methods are root-only; the local bus keeps group support. See libink/README.md for the note on lifting that. Shutdown: - Call dbus_exit() from the shutdown path so the server, its peers, and the socket are let go cleanly. The teardown existed but nobody called it. Tests, CI, docs: - A fuzz target for the message parser, run as a quick sweep in the suite and properly under libFuzzer in CI, with the corpus carried between runs. The -as-uid tests drop groups the way a login does so SO_PEERGROUPS sees the right set, and widen the test socket to reach the per-method check behind the 0660 gate. Bring the GitHub actions up to versions that run on Node 24, and tidy a few small things a /simplify pass turned up. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
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.
Finit 5.0 D-Bus support.