From 3047123f2b94544e05a6e866167a7e894de798a0 Mon Sep 17 00:00:00 2001 From: jknapp Date: Wed, 5 Aug 2026 14:08:09 -0700 Subject: [PATCH] docs(lsphp): correct three comments that overstated what the code does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found all three describing behaviour the code does not have: - The range bound does NOT prevent opcache's shared-memory startup failure. With 99-prod-overrides setting interned_strings_buffer=16, a memory_consumption of 8 or 16 is accepted here and still aborts opcache. Documented rather than raising the floor, which would forfeit the superset property. - memory_consumption's 4096 ceiling is ours, not PHP's — PHP imposes no upper bound on that directive. Only the max_accelerated_files range is a vendor clamp. Also records that an out-of-range value resets to PHP's COMPILED default, discarding the image's own override. - The stale-fragment rm -f is defensive, not a bug fix: changing these env vars requires a recreate, which starts from a fresh layer, so the scenario the comment described is not reachable via docker restart. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/entrypoint-lsphp.sh | 38 +++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/scripts/entrypoint-lsphp.sh b/scripts/entrypoint-lsphp.sh index 348d358..aeaf1f4 100644 --- a/scripts/entrypoint-lsphp.sh +++ b/scripts/entrypoint-lsphp.sh @@ -139,9 +139,15 @@ echo "Container memory: ${CONTAINER_MEMORY_MB}MB | PHP_LSAPI_CHILDREN=${PHP_LSAP ## ## Digits-only is what closes the injection: no newline, quote, `$` or `{` can ## survive it, so neither an ini-directive injection nor php.ini's `${VAR}` -## interpolation is reachable regardless of what the caller sent. The range -## bound is a separate concern — it stops a typo'd value from making opcache -## fail its shared-memory allocation at startup. +## interpolation is reachable regardless of what the caller sent. +## +## The range bound is a separate, weaker concern: it is a sanity check, NOT a +## guarantee that the value works. Measured — with `99-prod-overrides.ini` +## setting `opcache.interned_strings_buffer = 16`, a memory_consumption of 8 or +## 16 is ACCEPTED here and still aborts opcache at startup ("Insufficient shared +## memory for interned strings buffer"), loading no opcache at all. The floor is +## not raised to cover that because doing so would forfeit the superset property +## below; the panel clamps at 32, well clear of it. validate_ini_num() { local name="$1" val="$2" min="$3" max="$4" INI_NUM="" @@ -257,12 +263,17 @@ EOF ## `domain`, and the panel is a different repo on a different release cadence. ## Validate at the point of use, where the ini is actually generated. ## - ## The accepted ranges below are PHP's OWN limits for these directives - ## (opcache refuses memory_consumption under 8 MB and clamps - ## max_accelerated_files into [200, 1000000]), deliberately a strict SUPERSET - ## of the panel's clamps: a value outside them could not have taken effect - ## anyway, and widening a panel clamp later can never start silently - ## rejecting real sites here. + ## The accepted ranges below are deliberately a strict SUPERSET of the panel's + ## clamps (32-512 and 2000-32000), so widening a panel clamp later can never + ## start silently rejecting real sites here. + ## + ## Provenance, stated honestly: max_accelerated_files [200, 1000000] IS PHP's + ## own clamp. For memory_consumption, 8 is PHP's documented floor but 4096 is + ## OURS — PHP imposes no upper bound on that directive. It is a typo guard, not + ## a vendor limit. An out-of-range value is not merely ignored: PHP resets the + ## directive to its COMPILED default, discarding the image's own + ## `99-prod-overrides` value, which is a further reason to reject rather than + ## pass such a value through. OPCACHE_LINES=() if [ -n "${OPCACHE_MEMORY_MB:-}" ]; then validate_ini_num OPCACHE_MEMORY_MB "$OPCACHE_MEMORY_MB" 8 4096 @@ -284,9 +295,12 @@ EOF } > "$SCAN_DIR/99-user-opcache.ini" else ## Nothing valid to say. Remove rather than leave whatever a previous boot - ## wrote — the env is the source of truth and the container filesystem - ## outlives a `docker restart`. Without this, an override that is later - ## cleared (or rejected) would keep applying from the stale fragment. + ## wrote. Defensive only — do not read this as fixing a reachable bug: the + ## writable layer does outlive a `docker restart`, but so does the + ## environment, and changing these vars requires a RECREATE, which starts + ## from a fresh layer with no stale fragment. Kept because it is free, and + ## because it makes "no valid override" mean the same thing on every boot + ## regardless of how the container got here. rm -f "$SCAN_DIR/99-user-opcache.ini" fi else