fix(autotools): support pipe in install prefixes - #190
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: shell-safe Autotools prefix
The symlink-alias approach is a sound strategy for keeping the matrix | delimiter out of Autotools-generated shell recipes, and it preserves the physical output directory. The Lstat/type-check/Readlink guard is careful, error handling matches the file's panic convention, and the E2E test covers the happy path. A few points below are worth considering — none are blocking.
Context verified: installDir is workspaceDir/<escapedPath>@<version>-<matrix> (internal/build/cache.go), and the | originates as the require/options separator in Matrix.Combinations() (formula/classfile.go). So | is the one metacharacter the system itself injects — but the matrix keys/values, version, and workspace path that surround it are not validated for other shell-hostile characters.
Additional findings
x/autotools/autotools.go:67: [P3] Configure doc comment does not mention prefix aliasing: After this change, wheninstallDircontains|, the value passed as--prefixis silently rewritten to a.llar-prefixsymlink in the build dir, and a stray symlink is left behind (never cleaned up). TheConfiguredoc comment still only says--prefix is prepended automatically when installDir is set.This is an observable, operator-facing behavior change (the prefix recorded inconfig.log/generated Makefiles differs from what the caller passed toNew). Worth a line noting that a prefix with shell-hostile characters may be substituted with an in-build-dir symlink alias.
| prefix := a.installDir | ||
| // Keep the pipe character out of generated Autotools shell recipes | ||
| // without changing the physical output directory. | ||
| if strings.ContainsRune(prefix, '|') { |
There was a problem hiding this comment.
[P2] shellSafePrefix only handles '|', not other shell metacharacters
The guard triggers only on strings.ContainsRune(prefix, '|') and the method is named shellSafePrefix, but the class of problem is broader: Autotools bakes --prefix into generated Makefiles/libtool scripts that /bin/sh later evaluates, and those recipes also break on spaces, $, backticks, ;, &, <, >, quotes, #, etc. Today | is the only char the system guarantees to inject (the matrix separator), so the practical exposure is limited — but matrix keys/values, the version, and the workspace path all flow into this prefix unchecked. Consider either narrowing the name/comment to reflect the |-only scope (and why: configure uses | as a sed delimiter), or broadening the trigger to an allowlist of shell-safe characters so the alias fires for any hostile prefix.
| if err != nil { | ||
| panic(err) | ||
| } | ||
| alias := filepath.Join(workDir, ".llar-prefix") |
There was a problem hiding this comment.
[P2] Alias inherits shell metacharacters from workDir
alias := filepath.Join(workDir, ".llar-prefix") — the returned "shell-safe" prefix is only as safe as workDir (the build dir). If buildDir contains a | (or any other metacharacter), the alias inherits it and the produced --prefix is not shell-safe. The trigger at line 83 also only inspects installDir, never workDir, so a hostile build dir both bypasses the check and, when the alias does fire for an unrelated reason, yields a metacharacter-laden prefix. The alias mechanism's safety fundamentally depends on workDir being clean, which is never validated.
| if err != nil { | ||
| panic(err) | ||
| } | ||
| alias := filepath.Join(workDir, ".llar-prefix") |
There was a problem hiding this comment.
[P3] buildDir=="" drops .llar-prefix symlink into process cwd
workDir() returns "." when buildDir is empty, so a |-in-installDir build with no build dir creates a .llar-prefix symlink in the current working directory (typically the source tree) and leaves it behind uncleaned. Minor, but worth a cleanup or a note.
| if !filepath.IsAbs(target) { | ||
| target, err = filepath.Abs(filepath.Join(workDir, target)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
[P3] Relative installDir resolved against workDir diverges from OutputDir()
When installDir is relative, the symlink target is resolved as filepath.Abs(filepath.Join(workDir, installDir)), i.e. relative to the (absolute) build dir. But OutputDir() returns the raw relative installDir, which the OS interprets relative to the process cwd. If buildDir != ".", these two interpretations of the same relative path diverge, so files install under workDir/installDir while OutputDir() reports cwd/installDir. Callers appear to pass absolute paths today, but this is a latent inconsistency.
1 participant
Autotools builds can fail when the install prefix contains
|, because generated shell recipes interpret the character as a pipeline operator.The implementation includes:
|inAutoToolsinstall prefixes duringConfigure..llar-prefixsymlink inside the build directory while preserving the original physical output directory.This keeps existing output paths and ordinary prefix behavior unchanged while allowing Autotools projects such as libiconv to install successfully under matrix output directories.