Add --env and --clear-env launch flag support. - #298
Open
henrybear327 wants to merge 2 commits into
Open
Conversation
An OCI image config carries an environment the guest must start with, so
elfuse needs a merge that follows `docker run -e` rather than one that
approximates it. guest_env_build in src/core/guest-env.c is that merge,
kept out of main() so it can be exercised against base vectors no live
process could be given: the host environment arrives as a parameter, so
nothing in the module reads global state.
"KEY=VALUE" replaces that name where env_find() locates it and appends
otherwise; a bare "KEY" imports the launcher's value, appending nothing
rather than an empty value when the launcher does not set it; clear_env
drops the base while leaving a bare "KEY" resolvable. An empty variable
name is refused, as setenv(3) refuses it.
host_env overrides *out_envp
-------------- --------- ----------
[0] PATH=/bin PATH=/opt PATH=/opt replaced in place
[1] WEIRD dropped: no '='
[2] TERM=xterm TERM=xterm forwarded
[3] PATH=/dup dropped: dup of [0]
NEW=1 NEW=1 appended
An entry the overrides cannot address is dropped rather than forwarded:
entry_key_len() returns 0 for [1], and env_find() matches only [0] for
[3]. Forwarding either would leave the guest holding both an override
and the entry it was meant to replace. getenv(3) reads the first copy
either way, but anything walking environ directly, /proc/self/environ
included, would see the stale one.
free_guest_argv generalizes to strv_free in src/utils.h, which frees the
guest argv and a built envp alike.
tests/test-guest-env-host.c enumerates the merge as a cross product of
base x clear_env x override sequence, holding every cell to a reference
merge and to structural invariants no cell may violate. Its alphabet
carries a refused token, so a rejection lands at every base and either
override position rather than at one hand-picked base, and one base sets
a name to the empty string, so a bare "KEY" import separates a name the
launcher set to "" from one it never set. Both widen a coverage guard
rather than fix an observed failure. The oracle compares whole names
with strcmp where env_find compares klen bytes and then the boundary
byte, which is the axis it is independent on; it shares the drop and
refusal rules, so it rules out a mechanism bug rather than a wrong
reading of `docker run -e`.
`elfuse-oci run` launches every guest as `elfuse --clear-env --env K=V ... --`, so a front end needs to set the guest environment through the CLI rather than by patching the runtime. Both flags map onto launch_args_t beside --user and --workdir. The new launch_args_t envp field generalizes elfuse_launch's hard-coded environ on guest_env_build's rule: a NULL vector means the host environ, which is what leaves callers that set neither flag unchanged. main() collects the --env tokens as borrowed argv pointers into one argc-sized allocation and builds the vector before runtime_set_process_title clobbers the argv block, which is what lets the refusal name the offending token and keeps a malformed --env from reaching --create-sysroot. tests/test-launch-flags.sh adds only what the guest_env_build cross product cannot reach: main() collecting the tokens out of argv, build_linux_stack copying the vector onto the initial guest stack, and the /proc/self/environ sink, which is held to the same entries build_linux_stack copied rather than compared against another sink of one run. tests/test-env-dump.c is the guest that prints its environ for those lanes. docs/usage.md documents the import and replace rules and the empty-name refusal.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/usage.md">
<violation number="1" location="docs/usage.md:57">
P3: The new paragraph says "A contradictory `--user` or `--env` request is rejected". Unlike `--user`, which has a genuine contradiction (fakeroot plus a non-root uid), `--env` has no contradictory combination: the only rejection in `guest_env_build` is a malformed entry with an empty variable name (main.c calls this a "malformed --env"). Lumping `--env` under "contradictory" is inaccurate and may send a reader looking for a conflict that cannot exist. Reword to distinguish the malformed `--env` case.</violation>
</file>
<file name="tests/test-guest-env-host.c">
<violation number="1" location="tests/test-guest-env-host.c:291">
P3: Each fully-passing non-refused cell registers a pass twice and structural failures can register additional pass/fail counts for the same cell. `check_structure` calls `host_ok()` on success, then returns, but `run_cell` ignores the result and proceeds to the length/entry checks, calling `host_ok()`/`host_check(false)` again. `host_passes`/`host_fails` in host-test-util.h are single global counters, so the printed summary double-counts (e.g. ~2912 instead of ~1456 passes) and a single defective cell can report both a pass and a fail. Make `check_structure` return a bool and only run the length/entry comparisons when it passed, so each cell contributes exactly one verdict.</violation>
</file>
<file name="tests/test-launch-flags.sh">
<violation number="1" location="tests/test-launch-flags.sh:133">
P3: In `env_exact`, `guest_env()` runs `2> /dev/null`, so when the guest fails to launch the helper reports only `(exit $status)` with no stderr from elfuse and no reason, while the non-helper lanes (inherited/appended/replacement) consistently print the captured output on a miss. A launch failure in CI (wrong binary, entitlement, arch) is therefore opaque in the very lanes that assert the most. Capture the runtime output so the failure path can print it alongside the exit code.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| runs as something other than what was asked for. | ||
| `--user`, `--workdir`, `--env`, and `--clear-env` select what the guest starts as, | ||
| where it starts, and what it sees in its environment. A contradictory `--user` | ||
| or `--env` request is rejected before the VM is created, and `--workdir` is |
There was a problem hiding this comment.
P3: The new paragraph says "A contradictory --user or --env request is rejected". Unlike --user, which has a genuine contradiction (fakeroot plus a non-root uid), --env has no contradictory combination: the only rejection in guest_env_build is a malformed entry with an empty variable name (main.c calls this a "malformed --env"). Lumping --env under "contradictory" is inaccurate and may send a reader looking for a conflict that cannot exist. Reword to distinguish the malformed --env case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/usage.md, line 57:
<comment>The new paragraph says "A contradictory `--user` or `--env` request is rejected". Unlike `--user`, which has a genuine contradiction (fakeroot plus a non-root uid), `--env` has no contradictory combination: the only rejection in `guest_env_build` is a malformed entry with an empty variable name (main.c calls this a "malformed --env"). Lumping `--env` under "contradictory" is inaccurate and may send a reader looking for a conflict that cannot exist. Reword to distinguish the malformed `--env` case.</comment>
<file context>
@@ -48,13 +50,14 @@ only bounds a single `hv_vcpu_run()` iteration before the host regains control,
-runs as something other than what was asked for.
+`--user`, `--workdir`, `--env`, and `--clear-env` select what the guest starts as,
+where it starts, and what it sees in its environment. A contradictory `--user`
+or `--env` request is rejected before the VM is created, and `--workdir` is
+resolved during bring-up, before the first guest instruction, so a bad request
+fails with a diagnostic instead of launching a guest that runs as something
</file context>
| return; | ||
| } | ||
|
|
||
| check_structure(envp, n, cell); |
There was a problem hiding this comment.
P3: Each fully-passing non-refused cell registers a pass twice and structural failures can register additional pass/fail counts for the same cell. check_structure calls host_ok() on success, then returns, but run_cell ignores the result and proceeds to the length/entry checks, calling host_ok()/host_check(false) again. host_passes/host_fails in host-test-util.h are single global counters, so the printed summary double-counts (e.g. ~2912 instead of ~1456 passes) and a single defective cell can report both a pass and a fail. Make check_structure return a bool and only run the length/entry comparisons when it passed, so each cell contributes exactly one verdict.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test-guest-env-host.c, line 291:
<comment>Each fully-passing non-refused cell registers a pass twice and structural failures can register additional pass/fail counts for the same cell. `check_structure` calls `host_ok()` on success, then returns, but `run_cell` ignores the result and proceeds to the length/entry checks, calling `host_ok()`/`host_check(false)` again. `host_passes`/`host_fails` in host-test-util.h are single global counters, so the printed summary double-counts (e.g. ~2912 instead of ~1456 passes) and a single defective cell can report both a pass and a fail. Make `check_structure` return a bool and only run the length/entry comparisons when it passed, so each cell contributes exactly one verdict.</comment>
<file context>
@@ -0,0 +1,368 @@
+ return;
+ }
+
+ check_structure(envp, n, cell);
+
+ if (n != want.n) {
</file context>
| { | ||
| local desc="$1" want="$2" | ||
| shift 2 | ||
| local got status=0 |
There was a problem hiding this comment.
P3: In env_exact, guest_env() runs 2> /dev/null, so when the guest fails to launch the helper reports only (exit $status) with no stderr from elfuse and no reason, while the non-helper lanes (inherited/appended/replacement) consistently print the captured output on a miss. A launch failure in CI (wrong binary, entitlement, arch) is therefore opaque in the very lanes that assert the most. Capture the runtime output so the failure path can print it alongside the exit code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test-launch-flags.sh, line 133:
<comment>In `env_exact`, `guest_env()` runs `2> /dev/null`, so when the guest fails to launch the helper reports only `(exit $status)` with no stderr from elfuse and no reason, while the non-helper lanes (inherited/appended/replacement) consistently print the captured output on a miss. A launch failure in CI (wrong binary, entitlement, arch) is therefore opaque in the very lanes that assert the most. Capture the runtime output so the failure path can print it alongside the exit code.</comment>
<file context>
@@ -98,6 +111,150 @@ check accept "--workdir under a root sysroot" '' --sysroot / --workdir /var/tmp
+{
+ local desc="$1" want="$2"
+ shift 2
+ local got status=0
+ got="$(guest_env "$@")" || status=$?
+ if [ "$status" -ne 0 ]; then
</file context>
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
1 participant
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.
In preparation for the OCI support, we need to support the ENV flag.
Summary by cubic
Adds
--envand--clear-envlaunch flags to control the guest environment, followingdocker run -esemantics for upcoming OCI support. Previously the guest always inherited the host environment; now you can start from empty and set/import variables. Default behavior remains unchanged.guest_env_buildto merge the environment: KEY=VALUE replaces or appends; bare KEY imports the host value when set; rejects empty variable names;--clear-envstarts from nothing. Drops malformed base entries and duplicate names to avoid stale copies.maincollects repeated--envtokens and builds the vector before proctitle updates; rejects invalid entries early;--still ends elfuse option parsing.launch_args_twithenvp.elfuse_launchuses this vector when provided, or the hostenvironwhen NULL. Ownership stays with the caller.test-guest-env-hostcross-product oracle for the merge, new guesttest-env-dump, andtest-launch-flags.shlanes that verify stack and/proc/self/environ.--clear-env.Migration for embedders calling
elfuse_launchenvpfield. Setenvp = NULLto preserve old behavior (inherit host environment), or pass a malloc’d NULL-terminated vector and free it after launch (e.g., withstrv_free).Written for commit efbad6b. Summary will update on new commits.