fix(getenv): a variable that is not set answers false, not "" - #801
Conversation
PHP separates a variable that is NOT SET from one set to the empty string:
`getenv()` answers `false` for the first and `""` for the second, and every
"is this configured" check is written on that separation — `getenv($n) !== false`.
elephc collapsed them, so that test was true for every name. Silently: no error,
no warning, just the other branch.
The information was never missing. libc already distinguishes the two — a
missing name gives NULL, an empty value gives a valid pointer to a zero-length
string — and `__rt_getenv` already returned a null pointer for the first case.
What discarded it was the descriptor, which overrode the checker's `string|false`
to a plain `Str` "for present and missing variables alike". The checker had it
right the whole time; the EIR result now carries the same union, and the lowering
boxes it through `box_owned_string_or_false_result`, the convention `ob_get_clean`
and a dozen others already use.
The found path copies the value out of the environment block first. That is a
contract requirement rather than an observed crash: the result is boxed as an
OWNED string, and anything later classifying that payload reads its heap header
eight bytes before a string the allocator never handed out. Removing the copy
leaves every test green, because a foreign free is range-rejected — so the reason
is written where the copy is instead of asserted by a test that would pass
against its own bug.
One caller-visible consequence, which is the honest price of an honest type:
assigning the result into a local already typed `string` is now a type error
(`cannot reassign $x from string to string|false`). A fresh local is unaffected,
and the whole existing corpus compiles.
`test_getenv_nonexistent` kept passing against this for as long as it existed:
`strlen(false)` and `strlen("")` are both 0, so it pinned the length and never
the distinction. Its comment now says so, and the new test uses `=== false` in
both directions on one program — the empty-but-set case being what a fix in the
wrong place breaks. Mutation-verified: dropping the boxing reproduces the
original output exactly, `unset:string ... idiom:taken`.
The same shape exists in exactly one other builtin, `readline` (checker
`string|false`, descriptor narrowed to `Str`). Left alone deliberately: it reads
a line rather than an environment variable, PHP strips its newline where `fgets`
keeps it, and EOF is signalled differently — measuring that is its own change,
and bundling it unverified with this one would make neither trustworthy.
`elephc` lib 1488 · bins 1661 · error_tests 1437 · codegen callables 445 ·
ir_backend_smoke 257 · opcache_env_override 12. Output diffed against `php -n`:
identical on every getenv form.
Greptile SummaryThe PR restores PHP-compatible
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/builtins/system/getenv.rs | Removes the narrowing override so the checked `string |
| src/codegen/lower_inst/builtins/system.rs | Boxes the runtime pointer pair as an owned string or the exact PHP false value. |
| src/codegen_support/runtime/system/getenv.rs | Preserves libc's unset-versus-empty distinction and copies present values into owned storage on both supported architectures. |
| src/ir/runtime_fn.rs | Aligns getenv effects and fresh-result ownership with its new heap allocations. |
| tests/codegen/callables/constants_and_system.rs | Adds end-to-end regressions for unset and empty values plus result and temporary-name ownership. |
| scripts/docs/elephc_builtins/registry.py | Presents the checker-derived union accurately despite the neutral contract representation lacking unions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["PHP getenv(name)"] --> B["libc getenv"]
B --> C{"Pointer is NULL?"}
C -->|"Yes: unset"| D["Return null pair"]
D --> E["Box PHP false"]
C -->|"No: present"| F["Measure value length"]
F --> G["Copy into owned heap string"]
G --> H["Box PHP string"]
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
`Result type source: shared -> checked`, in the generated builtin page and the registry the generator writes beside it. That is exactly what dropping getenv's `eir_result_type` override means: the EIR result now comes from the checker's `string|false` rather than from a shared override narrowing it to `Str`. The CI gate that caught this regenerates and compares, so the fix is to run the generator, not to edit the page. `audit_builtins.py` and `gen_php_comparison.py` report no further drift.
|
CI caught a gate I had checked and misread: One line in the page, one in the registry beside it. Regenerated with The rest of that run was green (127 jobs); |
|
Updated this PR and pushed commit
Local validation completed successfully:
The GitHub Actions matrix has restarted and is currently running. |
PHP separates a variable that is not set from one set to the empty string:
getenv()answersfalsefor the first and""for the second. Every "is thisconfigured" check is written on that separation —
getenv($name) !== false—and elephc collapsed the two, so that test was true for every name. Silently: no
error, no warning, just the other branch taken.
Found by accident, which is the part worth reporting: a test program that was
supposed to fork ran entirely inside its child branch, and the first symptom
looked like a profiler failing to find any child processes. The symptom never
points at the cause.
The information was never missing
libc already distinguishes the two — a missing name gives NULL, an empty value
gives a valid pointer to a zero-length string — and
__rt_getenvalreadyreturned a null pointer for the first case. What discarded it was the
descriptor, overriding the checker's
string|falseto a plainStr"forpresent and missing variables alike".
The checker had it right the whole time. The EIR result now carries the same
union, and the lowering boxes it through
box_owned_string_or_false_result— theconvention
ob_get_clean,gethostbyaddr,realpathand a dozen others alreadyuse.
The found path copies the value out of the environment block first. That is a
contract requirement rather than an observed crash: the result is boxed as an
owned string, and anything later classifying that payload reads its heap header
eight bytes before a string the allocator never handed out. Removing the copy
leaves every test green, because a foreign free is range-rejected — so the reason
is written where the copy is, rather than asserted by a test that would pass
against its own bug.
Measured against
php -ngetenv('ABSENT')bool(false)string(0) ""bool(false)getenv('ABSENT') === falsetruefalsetruegetenv('EMPTY')(set to"")string(0) ""string(0) ""string(0) ""getenv('EMPTY') === falsefalsefalsefalsegetenv($x) !== false ? …Every
getenvform now matchesphp -nbyte for byte.One caller-visible consequence
The honest price of an honest type: assigning the result into a local already
typed
stringis now a type error —cannot reassign $x from string to string|false. A fresh local is unaffected, and the entire existing corpuscompiles. It surfaced in my own fixture while writing this, which is how I know
what it looks like.
On the tests
test_getenv_nonexistenthad pinned this for as long as it existed and neversaw it:
strlen(false)andstrlen("")are both0, so it fixed the length andnot the distinction. Its comment now says so. The new test uses
=== falseinboth directions on one program, because the empty-but-set case is what a fix in
the wrong place breaks. Mutation-verified — dropping the boxing reproduces the
original output exactly:
unset:string empty:string idiom:taken.The leak test says explicitly what it does not cover, rather than implying it.
The same shape exists once more, and is deliberately not here
readlinehas the identical descriptor narrowing (checkerstring|false,descriptor
Str); a sweep of every builtin that overrides its result type foundno others. It is left alone on purpose: it reads a line rather than an
environment variable, PHP strips its newline where
fgetskeeps it, and EOF issignalled differently — measuring that is its own change, and bundling it
unverified with this one would make neither trustworthy.
Two neighbours are also out of scope and worth their own issue:
$_ENVand$_SERVERare empty in CLI mode (noPATH, noargv;$_SERVERispopulated under
--web), andgetenv()with no argument and thelocal_onlysecond parameter are unimplemented — both loudly, as compile errors.
Suites
elephclib 1488 · bins 1661 ·error_tests1437 · codegen callables 445 ·ir_backend_smoke257 ·opcache_env_override12.fmtand clippy clean.