Repository navigation
Enforce the dinit generation and essential-service contract - #76
Merged
Merged
Conversation
checkTransition had no behavioral test. Route its host, init and dinitctl queries through a Probe so a test can supply fixed answers, and cover refusing an active removed service, allowing a stopped one, skipping an alternate root without querying dinit, and failing closed on malformed list or status output.
checkTransition asked whether dinit was running before it worked out what the transition removes. An unreadable /proc/1/comm with no /run/dinitctl then refused every activation on the host root, including ones that remove nothing and cannot affect a running service. Compute the removed set first and require a readable init identity only when something is removed.
The ownership test treated any loaded name starting with a removed definition and an '@' as an instance of it. A removed template such as getty@ never matched its own instance getty@tty1, so a running instance was not refused, while removing a plain foo was refused because an instance of a surviving template foo@ happened to start with 'foo@'. Add one classifier in dinit.zig used by the policy check and by the essential lookup, so that disabling an instance of an essential template is rejected like disabling the template.
Activation read the current generation's manifest before deciding whether the live service check applies at all. A damaged current generation then blocked activation on an alternate root and under other init providers, and blocked rolling away from the damaged generation. Gate the read on the host root under the dinit provider.
startService reloaded whenever is-started failed, which includes a service that is starting or stopping, where dinit can refuse a reload, and one dinit has not loaded, where start loads the fresh definition on its own. Reload only a service that dinit reports as stopped, and name the step that failed so a refused reload is not reported as a failed start. Run dinitctl through a replaceable runner so the tests can check the exact command sequence.
Activation turned every service tree failure other than out-of-memory into InvalidInput, so a permission problem or a disk error was indistinguishable from a bad service definition. Map each class to its own activation error and include the underlying error name when publishing the tree fails.
The tests covered the contents of the generated mere-essential file but not the mere-essential.d link that the boot definition reads through depends-on.d, nor the rejection of package services named mere-essential or mere-essential.d. Check that the link resolves to the bundle with its marker, and that both reserved names are refused.
Disabling an essential service, and the essential label in status and list, had no test. Let the service definition directories be redirected so the facade can run against a temporary tree, and cover refusing to disable an essential service, including one an administrator has overridden and an instance of an essential template, while an ordinary service still reaches dinitctl. The list test exposed a leak in listing dinit services: the name map was never released and an administrator override duplicated its name, so each listing leaked. Release the map, skip names already recorded, and free the returned slice once its names have moved into the entries.
Activation was only tested against an alternate root, where the service policy never applies. Let a test supply the host answers that activation consults, and cover rolling back to a generation that drops a running service being refused with the profile left unchanged, the same rollback being allowed once the service is stopped, an activation that removes nothing succeeding when live state cannot be read, and the target generation's service tree and essential bundle being published.
exec discarded the result of each dinitctl call without freeing its stdout and stderr, so every stop, start, restart, reload, enable and disable leaked. Listing services leaked the name map and a second copy of any name an administrator overrides, and a failure part way through leaked the names it had not yet handed to an entry. Scanning the definition directories swallowed allocation failures with catch continue and returned a partial list. Free the result in exec, release each scanned name exactly once whether the listing succeeds or fails, and report out-of-memory from the scan. Make the test runners return output as dinitctl does, since empty strings cannot leak and had hidden all of this, and add tests that fail each allocation in turn.
parseServiceArgs duplicated each command, up, down and depends-on argument and then appended it. If the append failed, the duplicate was never freed. Free it on that path, and add a test that fails each allocation in turn while writing the service tree, which is how the leak was found.
Fail each allocation made by checkTransition in turn and check that nothing is left allocated, whether the check succeeds or reports a refusal.
The s6rc code had the same defects as the dinit code it was copied from. Every exec leaked its stdout and stderr, ensureRepo leaked the result of probing for the repository, scanning the source directories swallowed allocation failures with catch continue and leaked a second copy of any name an administrator overrides, and a failure part way through listing leaked the names it had not yet handed to an entry or skipped. Free the result in exec and in ensureRepo, report out-of-memory from the scan and release each name exactly once, and make listing release skipped and unvisited names. A dependency name was also leaked when appending it to the status failed, and the dinit status path let an out-of-memory error from its queries be reported as a process failure; both now keep their real error and free what they allocated. Run the s6 commands through a replaceable runner that returns output the way the s6 tools do, and add tests that fail each allocation in turn for status and list under both providers.
addService and addRealizer copied each command, up, down, depends-on and input string and appended it in one expression, so a failed append leaked the copy. Route every such copy through one helper that frees it on failure, also used when parsing, and add tests that fail each allocation in turn for both.
…che key The split-staging report copied a conflicting path and an unassigned path and appended each in one expression, and the build cache key copied each companion file name the same way, so a failed append leaked the copy. Free the copy on that path, and add tests that fail each allocation in turn for both.
Scanning the key directories skipped a key whenever joining its path, computing its fingerprint or appending it ran out of memory, and moving the scanned keys leaked the ones not yet moved when an append failed. Collecting the trusted fingerprints for an import did the same on its own paths. A key silently dropped here makes the trusted set smaller than the configuration says, which can reject a validly signed package. Return out-of-memory from each of these paths and free what was allocated, and add a test that fails each allocation in turn while collecting the trusted fingerprints from a signing key and an installed key. The test catches the scan and move changes and the missing frees. It does not catch swallowing the failure on the signing-key path in the import code. Once one allocation fails the test allocator keeps failing, so a later allocation in the same call returns the error and the call never completes with a smaller set. That one conversion is covered by review, not by a test.
Parsing meta.kdl and populating recipe metadata copied each license and source URL and appended it in two steps, so a failed append leaked the copy, and assigned a description or url without freeing a value already set, so a repeated node leaked the earlier one. Building the properties JSON for an import turned every allocation failure into 'no metadata' and returned a half-built buffer to the allocator's mercy, which would have imported a package with its metadata silently missing. Route every copy through the helper that frees it on failure, free the value being replaced, and report out-of-memory from the properties JSON instead of returning null. Add tests that fail each allocation in turn for populating metadata and building the JSON, and one that checks a repeated node replaces the earlier value.
…lled one findTransitiveDependents dropped any edge or visited package it could not allocate for and returned the smaller set, so uninstall --cascade could leave a package installed whose dependency had just been removed. Return out-of-memory instead and free the result set if a later step fails. Two tests cover it: one fails each allocation in turn and checks nothing is left allocated, and one checks a call that still succeeds under an injected failure returns the same set as a clean run. Neither test catches a swallowed failure in the first two conversions (the reverse-map getOrPut and the edge append). Once one allocation fails the test allocator keeps failing, so a later put or append in the same call returns the error and the call never completes with a smaller set. Those two conversions are covered by review and by the other three, not by a test.
The comment said the cleanup checks for leaks but does not crash the test, which reads as leaks being tolerated. A leak through ctx.allocator is logged at error level and the leaked bytes are returned to std.testing.allocator, so the run fails; only the test body is not aborted. Say that.
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.
A generation switch can currently remove a running service, while later control commands can reuse an outdated dinit description. Refuse live transitions removing active package-owned services before activation, refresh stopped starts and explicit restarts from the active definition, and load the configured provider for CLI rollback. Upgrades and rollback leave existing processes running.
Generate a required
mere-essentialbundle from package metadata, show essential services in status, and reject ordinary disable. Prepare the complete service tree before switching generations, including rollback to older generations, while preserving administrator overrides. Alternate-root activation does not query host services; unavailable or unrecognized live state refuses a removing transition.Add focused regression coverage and document the contract, maintenance behavior, and explicit boot-configuration adoption. Distro boot wiring is a companion merelinux change.
Part of #63 and jhuntwork/merelinux#633.