build: isolate per-request process state - #2196
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: build: isolate per-request process state
This is a well-structured, well-tested refactor. Threading an explicit processSnapshot (env / working dir / PATH) through the build so Do no longer mutates global process state or the caller's Config is the right direction, and the new isolation tests (TestConcurrentBuildRequestsKeepInputsIsolated, TestResolveBuildConfigDoesNotAliasInput, TestDoDoesNotModifyConfigOnValidationError) exercise exactly the guarantees the PR claims. The frontendDebugMu RLock/Lock and llssaInitOnce patterns were reviewed and are correct: only the process-global diagnostic switches (cl.SetDebug/llssa.SetDebug) are serialized, and code-generation options are now genuinely request-local via cl.Options.
The main concern is a security regression in the new processenv.LookPath, detailed inline. The remaining items are maintainability nits.
Findings not tied to a single line
Six near-identical "last-wins environ lookup" helpers. The same reverse-scan strings.HasPrefix(env[i], key+"=") + environ == nil -> os.Getenv fallback is reimplemented in six places:
internal/processenv/processenv.goGet(exported, canonical)internal/build/request.goenvValueinternal/clang/clang.gogetenvinternal/env/env.golookupEnvxtool/env/llvm/llvm.gogetenvxtool/env/env.go(inline closure inexpandEnvWithCmd)
processenv.Get exists precisely for this. The three processenv importers (clang, llvm, xtool/env) could delegate to it; internal/env/internal/build may have import-cycle constraints worth checking. This duplication tends to drift and undercuts the maintainability goal of the refactor.
Variadic-config helpers add silent-misuse risk with no call-site benefit. cacheEnabled(conf ...*Config), plan9asmDisabledByEnv(confs ...*Config), and plan9asmEnabledByEnv(pkgPath, confs ...*Config) are each now called with exactly one *Config; the zero-arg form only preserves the legacy os.Getenv fallback. Since isEnvOnConfig/envConfigValue already treat a nil *Config as "fall back to os.Getenv", a plain *Config parameter called with nil in the legacy path would be clearer and would stop cacheEnabled(a, b) from compiling while silently ignoring the second arg.
Minor / documentation
cl/compile.goEnableExportRenamedoc: the retained "This is enabled when using -target flag..." sentence is now stale — the-targetbehavior flows throughOptions.ExportRename(set inbuild.go), not this deprecated setter.internal/processenv/processenv.goCommanddoc: "A nil environ preserves the standard os/exec process-inheritance behavior" is imprecise — whenenviron == nilthe function still forcescmd.Dir = dir, so the working directory is not left to os/exec defaults. Harmless in current build usage (dir is always supplied), just a wording nit for an exported helper.
|
|
||
| // LookPath searches file using PATH from environ. Relative PATH entries are | ||
| // interpreted from dir, which is where the resulting command will run. | ||
| func LookPath(environ []string, dir, file string) (string, error) { |
There was a problem hiding this comment.
Security: this LookPath reintroduces the CWD / relative-PATH execution risk that Go 1.19+ exec.LookPath guards against.
Since Go 1.19 the stdlib exec.LookPath deliberately refuses to run a bare command name resolved through a relative PATH entry (including the empty entry meaning "current directory"): it returns the path together with exec.ErrDot so the caller must opt in explicitly. This reimplementation does the opposite:
- an empty PATH entry is silently rewritten to
.(line 63-64); - any relative PATH entry is joined against
dirand used directly (line 66-67); - on a match it returns the candidate with a
nilerror (line 71-72) — neverErrDot.
Because dir is normally the build's working directory (snapshot.Dir = os.Getwd() or BuildRequest.Dir, i.e. the source tree being built), a clang / go / pkg-config / ar / tar / xcrun / llvm-config binary planted inside an untrusted checked-out source tree can be executed instead of the real toolchain — e.g. Command([]string{"PATH=bin"}, workDir, "clang") runs workDir/bin/clang. This is exactly the multi-tenant "per-request driver" scenario the PR is built to enable, so I'd treat it as blocking.
Related, in Command (line 34-36): exec.Command already runs the stdlib LookPath and would set cmd.Err = ErrDot for a .-resolved name, but this code then unconditionally overwrites cmd.Path/cmd.Err with the custom result, clearing that guard even for cases the stdlib blocked.
Recommendation: mirror stdlib semantics — when a match comes from a relative/empty PATH entry, return exec.ErrDot (or refuse) and propagate it through Command so callers opt in explicitly. The test at internal/processenv/processenv_test.go currently locks in the unsafe relative-PATH-from-CWD behavior and would need updating.
| return "", exec.ErrNotFound | ||
| } | ||
|
|
||
| func executable(path string) bool { |
There was a problem hiding this comment.
Minor divergence from stdlib LookPath that amplifies the ErrDot issue above: executable accepts any file with any executable bit set (Mode()&0o111 != 0), so a file executable only by group/other is treated as runnable even when the current user can't execute it, and os.Stat follows symlinks. The stdlib uses an access-based check (unix.Eaccess). This widens the set of candidate binaries selected during resolution. The IsDir() guard is correct.
| return isEnvOn(llgoBuildCache, true) | ||
| } | ||
|
|
||
| func isEnvOnConfig(conf *Config, key string, defVal bool) bool { |
There was a problem hiding this comment.
isEnvOnConfig re-encodes the truthiness set (1/true/on) that isEnvOn (just above) already implements as envVal == "1" || envVal == "true" || envVal == "on". They agree today but must be kept in sync by hand — and the whole point of this PR is that config-scoped and process-scoped env reads produce identical results. Consider funneling both through a single parseEnvBool(value, defVal) helper.
| return "", false | ||
| } | ||
|
|
||
| func withEnv(environ []string, values ...string) []string { |
There was a problem hiding this comment.
withEnv silently drops any environ entry lacking = (only strings.Cut(..)==ok entries are re-appended). In practice os.Environ() never yields such entries (Windows =C:=... drive entries still contain =), so this is effectively safe and looks intentional as normalization — but the behavior is silent and differs from a naive append. A one-line comment stating that non-KEY=VALUE entries are intentionally discarded would prevent a future "where did my env var go" investigation.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
63b6887 to
afc2d4c
Compare
afc2d4c to
522176e
Compare
1 participant
Summary
This is prerequisite cleanup for #2174 and the later package-parallel build work. It intentionally does not enable package parallelism yet.
Tests