Repository navigation
Craftsmanship hardening sprint: security, resilience, coverage, CI gate, docs - #217
Merged
Merged
Conversation
Gate 1: PASS (1 reviewer, C memory-safety lens) - 0 CRITICAL, 0 MAJOR,
3 MINOR, 4 NOTE; comment fixes applied in passing, follow-ups F-G1-1..4
recorded in plan Appendix A. Validation at gate: 240/240 PASS
(234 baseline + 6 new), 0 compiler warnings.
Fixes and red-runs (Rule 3):
1.1 Append guard + exception text now enumerate all 7 string-keyed
types incl. STRING_TO_ENTRY ($j[]=1 was SIGSEGV, exit 139).
Red-run: FAIL (tests/string_to_entry_append_001, crash pre-fix;
string_to_int_005 / string_to_mixed_005 EXPECTs updated to the
7-type message).
1.2 forEach/filter/map on STRING_TO_ENTRY no longer read a
judy_cache_entry_t* as zval*: routed through judy_value_from_slot().
Red-run: FAIL (tests/string_to_entry_callback_iterator_001,
exit 139 pre-fix; pins expired-entry -> NULL, Gate-1 ruling (b)).
1.3 equals() gains an ENTRY branch: zend_is_identical over values
materialised via judy_value_from_slot(), expired reads as NULL on
both sides (Gate-1 ruling (a)).
Red-run: FAIL (identical ENTRY arrays returned false / latent bad
deref; tests/equals_string_to_entry_001).
1.4 set() enforces PHP_JUDY_MAX_LENGTH before slot acquisition, with
increment()'s exact message. A 70 000-byte key was stored, then the
next ordered traversal copied it unbounded into the 64 KB
key_scratch (heap overflow).
Red-run: FAIL (observed: 'set() accepted oversized key', count=1,
keys() returned the full 70 000-byte string through key_scratch).
1.5 increment() captures key_len before zend_string_release (UAF read
of ZSTR_LEN in the exception message). RULE-3 EXCEPTION: the .phpt
cannot fail pre-fix on standard Zend MM (free-list header occupies
bytes 0-15, ZSTR_LEN at offset 16, no allocation in between) -
shipped as characterization; detector is source review (valgrind
unavailable, Step 3.7 SKIP recorded). Observed pre-fix: PASS.
1.6 increment() boundary arithmetic wraps via (zend_ulong) add in both
branches - removes signed-overflow UB at PHP_INT_MAX/MIN.
RULE-3 EXCEPTION (characterization): the red-run FAIL was an
incorrect initial EXPECT (INT_MIN-2 wraps to ...806, not ...805);
with the corrected EXPECT the pre-fix build already produced the
pinned wrap semantics (observed). Gate default ruling: WRAP, a
throw would need explicit sign-off (plan Step 1.6 / test comment).
Canonical ENTRY/expiry contract ruling recorded at Gate 1 (equals,
traversal visibility, count() pre-prune wording, mergeWith TTL/flags) -
plan Gate-1 record; cited by 1.3/3.3/3.6/5.2, never re-decided.
Gate 2: PASS (1 reviewer, vacuous-test lens) - 0 CRITICAL, 0 MAJOR,
1 MINOR, 4 NOTE; fixes applied in passing. Reviewer independently
extracted all 11 FILE sections (byte-exact outputs) and ran 6 mutation
simulations proving each pin fails on regression (arg-order flip,
throw-at-visit-3, silent set-op merge, wrong expected message,
inclusive searchNext, display_errors=0). Validation: 250/250 PASS
(234 baseline + 6 Batch-1 + 10 new), scope = tests/ only, no source
edits.
New pins (pass on arrival by design - they pin CURRENT behavior;
non-vacuity is the Gate-2 mutation evidence recorded in the plan):
2.1 callback_arg_order_001: forEach/filter/map hand ($value, $key)
on all 11 types - values deliberately distinct in type AND value
so a swap cannot alias into a false OK (docs are backwards; fixed
in Batch 5). callback_exception_001: exception propagates,
iteration stops (visits=2), source intact, object usable after;
3 storage classes x 3 ops.
2.2 set_ops_mixed_type_throw_001: union/intersect/diff/xor throw the
exact 'BITSET and integer-valued' message on same-type
INT_TO_MIXED / INT_TO_PACKED / STRING_TO_MIXED / STRING_TO_ENTRY
operands, both operands untouched (12 previously unpinned cells +
ENTRY). average_values_contract_001: empty->NULL for every type,
hand-computed means (15.0/1.5), BITSET presence mean, throw cells
packed/mixed/entry for averageValues, plus the sumValues() throw
cells missing from sum_values_001 including the empty-array
asymmetry (avg NULL vs sum THROW).
2.3 bounds_type_error_001: string bounds on int-keyed deleteRange()
(engine-style TypeError naming the argument) and int bounds on
string-keyed slice() (explicit C TypeError); silent-coercion
cells deliberately NOT pinned (bug N1 gets its failing
expectation in Step 3.8). nav_boundary_001: searchNext()
exclusivity, NULL (not false) past-end, first("")/searchNext("")
inclusive/exclusive; last("")/prev("") divergence deliberately
NOT pinned (recorded follow-up).
2.4 bycount_empty_slot_matrix_001: documented nulls for byCount +
empty-slot methods on the 3 string-keyed INT types; full matrix
on INT_TO_PACKED (zero prior coverage); lastEmpty()=-1 semantics
pinned with the '-1 stored -> -2' proof it is a real search.
judy_type_negative_001: TypeErrors on int/string/stdClass,
int 0 for newInstanceWithoutConstructor (stable over 500 runs).
2.5 string_to_entry_error_matrix_001: NUL rejection + wrong-receiver
TypeErrors for get/getEntry/getExpiry/getFlags (real get_class()
asserted after the Gate-2 MINOR), getAll/jsonSerialize happy
paths, increment() throw on both ADAPTIVE types.
unserialize_malformed_data_001: valid type + non-array data
rejects; comment carries the pending-main-checkout-diff caveat.
serialize_error_001: %A tightened to the exact blank line +
exact Warning line (%s path, on line 23) + exact throw line.
Batch-2 rule honored: no source file changed. Step 4.3 disclosure on
record in the plan Gate-2 record: the pinned Warning is emitted by
JTYPE() before the invalid-type throw, and the other session's pending
__unserialize range-check would throw before JTYPE - that diff landing
fails serialize_error_001 on the missing Warning line.
3.1 get_gc: collect STRING_TO_ENTRY stored values so value cycles are
collectable (private cursor; expired entries exposed to GC deliberately).
3.2 deleteRange() string-keyed ENTRY branch now frees the cache-entry struct
(trie/adaptive/hash) — pre-fix it leaked the struct AND its value refcount
and never ran __destruct. Delete-before-free ordering preserved.
3.3 Expired entries skipped by value-serving traversals (iterator, get,
keys/values/toArray) while count()/size() stay raw-counter O(1) — the
Gate-1 canonical ruling, applied consistently via judy_entry_is_expired_at.
3.4 clone() fail path: partial-populate errors now free state and throw via a
goto fail label (clone of ZEND_CLONE semantics); populate JERR guards +
JUDY_UNLIKELY; fixed a BITSET fromArray fall-through missing return.
Rule-3 exception: JERR is not portably inducible — success-path pins +
Gate-3 source review carry the coverage.
3.5 pruneExpired() re-entrancy + counter discipline: private emalloc cursor
(freed incl. on early exit), JSLD-confirmed removals gated on Rc == 1
before counter--/bytes_sub/free, entry struct freed BEFORE the value
destructor via ZVAL_COPY_VALUE steal. Measurement verdict: no per-entry
leak — the throw path leaves a bounded ~3560 B Zend-MM plateau (identical
at 1000 and 3000 rounds) plus ~57 B/round maxRSS watermark; pre-fix
1768xN figures were peak-waterline retention, not leaks.
3.6 mergeWith() STRING_TO_ENTRY >> ENTRY now copies expires_at/flags verbatim
(new judy_write_entry_verbatim(): struct copied via slot_acquire, value
addref'd before any user code, publish-then-destroy on overwrite) instead
of zeroing them through the generic write helper. Expired sources stay
hidden on read in the target, mirroring slice().
3.7 valgrind dynamic spot-check: SKIP-RECORDED (no valgrind on this machine);
the Step 1.5 UAF fix keeps source-review + .phpt characterization coverage.
3.8 Wrong-category range bounds now throw TypeError instead of silently
coercing to a wrong-range read: int-keyed keys/values/toArray/size/slice
reject non-numeric strings (keys("a","b") returned [0] before);
string-keyed deleteRange rejects integer bounds (Z_PARAM_STRING coerced
ints to their string form). Numeric strings still name integer keys,
matching deleteRange()'s Z_PARAM_LONG weak-mode contract.
Gate 3 (expert review, security/C-memory-safety lens): APPROVE, 0 CRITICAL,
0 MAJOR. Fixed both actioned MINORs: F-G3-1 all deleteRange struct-frees now
gated on the delete Rc == 1 (JERR is truthy and means the slot still owns the
value — a latent UAF; BITSET counter gated too); F-G3-2 mergeWith ENTRY copy
FAILURE now throws instead of silently dropping the entry. Manifest sync: 23
missing test <file> entries added to package.xml <contents> (16 tracked + 7
new), <release> untouched, XML re-validated.
Tests: 257/257 PASS on default and --enable-judy-debug-mirror builds,
0 compiler warnings both. check_diff clean. Disclosure recorded (Gate 3, out
of 3.6 scope): filter()/map() still strip ENTRY TTL/flags through the generic
write helper — value transforms, not copies; future-release decision.
Docs & observability hardening with Gate-5 expert-review findings fixed in-tree (all verified against code or a runtime probe of modules/judy.so): - forEach()/filter()/map() callback order documented as ($value, $key), matching judy_callback_iterator (args[0]=value) and the pinned tests - Compatibility matrix: 11th STRING_TO_ENTRY column added; the STRING_TO_INT_ADAPTIVE set-ops cell corrected to 'yes' (runtime-verified, pinned by tests/regression_adaptive_setops_001.phpt); STRING_TO_ENTRY row added to the Key/Value type table; footer now says the *unbounded* count()/size() forms are raw-counter O(1) — a bounded size() walks the key index (judy_count_range) and likewise includes expired-but-unpruned - generate-api-docs.php --check now also asserts every matrix row carries exactly matrix_columns with legend vocabulary, so a missing cell fails instead of rendering a silent '?' (red-run proven: dropped-cell -> exit 1) - SECURITY.md: 2.7.x supported / <2.7 unsupported, now shipped via package.xml <file role=doc> - docs/index.html: hero v2.7.1, PHP 8.1-8.5 + 8.6 prerelease, fictional --with-expanse removed everywhere; Expanse described accurately as an optional pure-Rust libJudy engine linkable via --with-judy=DIR - package.xml 2.7.0 changelog: --with-expanse claim corrected to the real --with-judy=DIR story - README.md '2.6x faster' pruneExpired claim reworded qualitatively; BENCHMARK.md version stamps reconciled (measured 2.4.2, re-verified through 2.5.2; vendoring arms measured separately on 2.6.0), tail fixed - CONTRIBUTING.md file-home table now lists shipped vs repo-only docs (AGENTS.md/CLAUDE.md/CONTRIBUTING.md/llms.txt repo-only) - examples/cache-ttl-pruning.php teaches the Gate-1 ruling truthfully: reads and traversals hide an expired entry; only count()/size() raw counters include it until pruneExpired() (example re-run, output matches) - Judy_arginfo.h regenerated (stub hash stamp only) after the stub docblock edit so the CI regenerate-then-diff arginfo gate stays green allow-agent-instructions: AGENTS.md doc-accuracy fix: empty-slot variants exist on all types, return null on string-keyed (verified in C + runtime probe)
Step 6.2 3-reviewer changeset review (all APPROVE, 0 CRITICAL/MAJOR). The only actionable findings were two Reviewer-C package.xml formatting MINORs, folded into this wrap-up commit per the reviewers' own guidance: - restore the trailing newline at EOF (batch 5's changelog edit had dropped it) - align the tests/ini_string_maxlength_001.phpt <file> entry indent (10 -> 9 spaces) No content change: xmllint clean, <release>/<version>/<date>/<stability> tokens byte-identical to main. Verified: discipline PASS (overrides: 1).
Test Results
Benchmark Results
Run Status
Memory (Linux, PHP 8.6)Peak RSS of a child process building one structure, n = 500,000, minus an empty-process floor. Judy's memory advantage is type- and scale-dependent — see BENCHMARK.md for the curve across sizes on a dedicated host.
API Speedups (Linux, PHP 8.6)Speedup = PHP time / Judy time. Bold = Judy faster.
Core Types (Linux, PHP 8.6)Speedup = PHP time / Judy time. Bold = Judy faster.
All types detailCore Types (Linux, PHP 8.6) — AllSpeedup = PHP time / Judy time. Bold = Judy faster.
Cross-version detail (Linux)Core Types — LinuxEach cell: Judy ms (speedup vs PHP array).
API — Linux
Cross-version detail (Windows)Core Types — WindowsEach cell: Judy ms (speedup vs PHP array).
API — Windows
Windows results (PHP 8.6)Memory (Windows, PHP 8.6)Peak RSS of a child process building one structure, n = 500,000, minus an empty-process floor. Judy's memory advantage is type- and scale-dependent — see BENCHMARK.md for the curve across sizes on a dedicated host.
API Speedups (Windows, PHP 8.6)Speedup = PHP time / Judy time. Bold = Judy faster.
Core Types (Windows, PHP 8.6)Speedup = PHP time / Judy time. Bold = Judy faster.
All types detailCore Types (Windows, PHP 8.6) — AllSpeedup = PHP time / Judy time. Bold = Judy faster.
Release Comparison (2.6.0 → 2.7.1)
Summary: 0 regressions, 84 unchanged, 16 not evaluated (unstable)
16 benchmarks not evaluated (within-arm scatter exceeded the effect being claimed)
Memory ComparisonMemory: 0 regressions, 10 unchanged
Full benchmark outputLinux — PHP 8.6Windows — PHP 8.6 |
This was referenced Oct 7, 2026
orieg
added a commit
that referenced
this pull request
Oct 8, 2026
…d nightly (#223) The secondary assertion in tests/string_to_entry_delete_range_001.phpt measured a memory_get_usage() delta around deleteRange() and required it to be > 0. Under valgrind memcheck the delta came out 0: freed zvals land on the Zend MM freelist, and memory_get_usage() does not reliably drop, while the Judy trie (and its held zvals) is invisible to it entirely. The nightly valgrind job failed its first post-#217 run on this (issue #222), even though the primary leak assertion -- __destruct fires 1000/1000 -- passed and valgrind reported 0 definite leaks. Replace the reading with Judy::memoryUsage(), the extension's own string-keyed accounting (approx_payload_bytes), which deleteRange() decrements by every confirmed removal and which is independent of the allocator: 38000 -> 0 deterministically on this test. Test identity (--TEST-- title, file name) is unchanged, so the test-floor ratchet baseline in reports/junit.xml still matches.
orieg
added a commit
that referenced
this pull request
Oct 8, 2026
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.
Craftsmanship hardening sprint — 5 batches, all gates green
Hardening only: no new API surface, no behavior change except verified bug fixes, each
shipped with a
.phptthat fails on the pre-fix code. 47 files, +2605/−199. Every batchgate (1 reviewer) and the final 3-reviewer changeset review (lenses: C memory-safety,
test discipline, docs/house-rules) returned PASS / APPROVE with 0 CRITICAL, 0 MAJOR.
Commits
2b0643eincrement()oversized-key message reports true length; rejection covers every key-taking method incl. range bounds56c82d6judy.debug_preview_sizecontract, byCount empty-slot matrix, bounds TypeErrors, judy_type() negatives, increment boundary arithmetic, callback arg order + exception propagation, adaptive counter zero-value, set-ops mixed-type throw, debug-info dynamic-property non-shadowing41758afcount()/size()raw counters include them untilpruneExpired()(verified + pinned);mergeWithpreserves ENTRY TTL/flags verbatim + mixed-refcount + reentrancy safety; deleteRange/pruneExpired destructor-reentrancy safety; GC cycles through MIXED and ENTRY values collectable;equals()on ENTRY compares payloadse0b6b8djudy.string.maxlengthINI cap (with clamp-helper fix caught by the gate probe); NULL-pointer inits on navigation branches; CI arginfo gate corrected — php-srcgen_stub.php --verifydoes not catch stub↔arginfo drift (it rewrites the header and only checks aliases); gate is now regenerate-then-diff and fails on drift (red-run proven)88124d8STRING_TO_INT_ADAPTIVEset-ops cell corrected toyes(doc aligned to existing pinregression_adaptive_setops_001); STRING_TO_ENTRY added to Key/Value table;generate-api-docs.php --checknow asserts matrix row-completeness + cell vocabulary (sabotage red-run proven); "Rust Expanse"/--with-expansefiction stripped (config.m4only defines--with-judy; Expanse is a real sibling libJudy-compat engine linked via--with-judy=DIR);Judy_arginfo.hregenerated (stub-hash stamp only); cache-ttl-pruning example corrected to verified expiry truth; BENCHMARK/CONTRIBUTING/SECURITY reconciled9991cd6<file>indent) — hygiene onlyValidation
phpize --clean: 0 compiler warnings (CI-exact source-location pattern).examples/*.phpexit 0. One documented caveat:dedup-large-stream.phpspawnschildren with a
JUDY_SOenv flag, so the plain smoke exercises its graceful fallback;re-run with
JUDY_SOset verifies the full measurement path (array/judy/hashed agree onunique: 266396).discipline check: PASS (27 gates, 0 errors, 1 override — see below).php scripts/generate-api-docs.php --check: PASS.Disclosures (taken as read by the 6.2 reviewers)
1.5 class-instance UAF (source-review detector), 1.6 increment wrap-vs-exhaustion choice
(characterization), 3.4 JERR not portably inducible, 3.5 leak verdict was no leak.
__unserializenits (report-only): the main-checkout holds an in-flight__unserializestrict-validation edit from another session; 3 nits + an ordering hazardwith
tests/serialize_error_001.phpt's pinned Warning were reported to its owner. ThisPR does not touch that path.
judy_string_slot_acquire;set()TTL signed-overflow; bulk half-population on a badkey; hand-rolled callback materialisation; runtime phpt that probes every
api-metadata.phpmatrix cell; duplication inventory D1–D7 (no consolidation ordered);stale README md5sums;
optimizeIteration-inheritance test deferred.libjudy/,baselines/,php_judy.hPHP_JUDY_VERSION, andpackage.xml<release>/<version>are byte-identical to main..claude/*and are not part of this PR.AGENTS.mdedit in batch 5 ships with anallow-agent-instructions: AGENTS.md <reason>directive in its commit body (and here)— the local MCP discipline tool shows a residual finding only because it evaluates
without the PR description; the CLI engine (what CI runs) honors the directive and passes.
Fixes #117