config: debuggable validation errors (JSON Pointer + suggestions)#3
config: debuggable validation errors (JSON Pointer + suggestions)#3
Conversation
Add planning documents covering the fork's direction and priorities: Roadmap docs: - README.md — index and navigation hub - unit-roadmap.md — cross-cutting platform work, core daemon, governance - unit-maintainer.md — maintainer-facing synthesis, priorities, backlog - unit-php.md — PHP ZTS worker pool, persistent worker, TrueAsync - unit-python.md — free-threaded 3.13t, subinterpreters, ASGI/WSGI - unit-ruby.md — thread pool, Ractors, Fiber scheduler, YJIT - unit-cron.md — scheduler/cron primitive for framework tasks - unit-arm32.md — armv7/armhf SIGBUS/alignment investigation - unit-todos.md — ~90 TODO/FIXME/HACK markers inventory - unit-wasm.md — WASM backends, WASI component model, OCI distribution Core changes: - nxt_conf.h — add new config validation helpers - nxt_conf_validation.c — expand validation for routes, targets, TLS - nxt_controller.c — wire up new validation entry points Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Detailed implementation plan for debuggable config errors: JSON Pointer paths + "did you mean" suggestions in validation error responses, fully additive to the existing response shape. Includes mermaid diagrams, file-level touch list, and the test matrix for test/test_config_validation_errors.py.
Implements roadmap D5. Config validation errors now carry an RFC 6901
JSON Pointer to the offending location (location.path) and, when a member
name is a close typo of a known schema key, a "did you mean" suggestion
(suggestion). Strictly additive: existing response fields and error
message wording are unchanged.
* src/nxt_conf.h: extend nxt_conf_validation_t with a stack-allocated
path linked list and two rendered output fields (pointer, suggestion).
Not exported.
* src/nxt_conf_validation.c:
- nxt_conf_vldt_render_pointer walks the path linked list, escapes
'~'->'~0' and '/'->'~1' per RFC 6901, and writes into vldt->pointer.
Always produces a non-NULL pointer; "" represents the document root.
- Folded into nxt_conf_vldt_error so every error call-site gets a
pointer for free, no message-text changes.
- Push/pop path segments at the three recursion sites:
nxt_conf_vldt_object, nxt_conf_vldt_object_iterator, and
nxt_conf_vldt_array_iterator (decimal index).
- nxt_conf_vldt_edit_distance: Damerau-Levenshtein with bounded-size
stack buffers (NXT_CONF_VLDT_EDIT_MAX = 64) - no heap, no dynamic
stack, portable alignment on armv7.
- nxt_conf_vldt_unknown_member: picks a uniquely-best close candidate
and stashes it in vldt->suggestion before calling the unchanged
"Unknown parameter" error.
* src/nxt_controller.c: nxt_controller_response_t gains pointer and
suggestion fields; the two validation failure sites copy them; the
response serializer emits location.path and top-level suggestion when
non-empty. The legacy location.offset/line/column path is untouched.
* test/test_config_validation_errors.py: ten cases covering path
threading at root, containers, array indices; RFC 6901 escaping;
suggestion thresholding; and backward-compat guards on the legacy
error shape.
andypost
force-pushed
the
claude/improve-config-parsing-nQ9bV
branch
from
0f52727 to
aa0e4ce
Compare
|
@gemini-code-assist does this bring any security concerns and review it as architect /gemini summary |
andypost
left a comment
andypost
left a comment
There was a problem hiding this comment.
📋 Review Summary
This PR implements a PHP Runtime Statistics API but contains a CRITICAL out-of-bounds memory access in the status reporting loop and lacks thread-safety for PHP ZTS builds. While the data structure is well-aligned for multiarch, the current architectural implementation results in system instability (SIGSEGV) and requires significant refactoring.
🔍 General Feedback
- Memory Safety: The status aggregation loop in
src/nxt_status.caccesses memory out of bounds, directly leading to the segmentation faults observed during testing. - ZTS Compatibility: Direct access to Zend globals in
src/nxt_php_status.hwithout proper TSRM handling will cause failures in Thread Safe builds of PHP. - Architectural Patterns: The use of
static inlinefunctions with nested#includestatements in header files is non-idiomatic and creates fragile build-time dependencies.
Inline Comments
File: src/nxt_status.c
[CRITICAL] Out-of-bounds access in nxt_status_get. The loop manually populates app_obj but incorrectly calculates or initializes the apps array members, causing SIGSEGV.
File: src/nxt_php_status.h
[HIGH] Non-idiomatic use of #include "ZendAccelerator.h" inside a function. This should be moved to the top-level with appropriate guards.
[HIGH] Lack of ZTS safety. Accessing ZCSG or EG globals requires TSRMLS_CC or ts_resource in ZTS builds.
|
Hello @andypost, thank you for the request. I have reviewed the architectural changes and the implementation plan for roadmap item D5. Security AssessmentFrom an architectural perspective, the changes are low risk regarding security:
Architectural Review
Overall, the design is sound, follows the existing patterns in /gemini summary |
…idation Addresses issues identified during code review of the D5 roadmap implementation: 1. Prevent potential `size_t` integer wraparound in JSON Pointer length calculation on 32-bit platforms, jumping directly to the failure block if length bounds are exceeded. 2. Explicitly set `pointer.start = NULL` upon allocation failure or size bounds violations. Previously, allocation failures kept `pointer.start = ""` (root), which masked the true failure location with a false root pointer. 3. Improve precision of JSON Pointer paths for `NXT_CONF_VLDT_REQUIRED` and `NXT_CONF_VLDT_TSTR` errors by pushing the parameter name to the `vldt->path` stack before generating the error, rather than attributing the error to the parent container. Co-authored-by: Gemini 3.1 Pro <gemini@google.com>
andypost
force-pushed
the
claude/improve-config-parsing-nQ9bV
branch
from
408b446 to
366eaf7
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.gh.lumenfield.work/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces more descriptive configuration validation errors by adding RFC 6901 JSON Pointers and typo suggestions based on Damerau-Levenshtein distance. The implementation involves extending the validation structure with a path stack, adding a pointer renderer, and updating the controller's response serialization to include new location and suggestion fields. A review comment suggested improving the encapsulation of the edit distance macro, while another comment regarding the usage of a boolean flag was found to be factually incorrect based on the provided code.
| * Used only inside validation error paths; input lengths are bounded by | ||
| * configuration member-name lengths. | ||
| */ | ||
| #define NXT_CONF_VLDT_EDIT_MAX 64 |
There was a problem hiding this comment.
The macro NXT_CONF_VLDT_EDIT_MAX is defined but only used within this file. If this is not intended to be used elsewhere, consider moving it closer to its usage or making it a static constant to improve encapsulation.
There was a problem hiding this comment.
Keeping the #define. This matches Unit's established pattern for file-local size caps — see NXT_HTTP_ERROR_LEN in src/nxt_http_error.c and UNKNOWN_STATUS_LENGTH in src/nxt_h1proto.c, plus the neighboring use of NXT_MAX_ERROR_STR and NXT_INT_T_LEN as stack-array dimensions in this same file (nxt_conf_validation.c).
A couple of reasons against changing it:
static const size_t NXT_CONF_VLDT_EDIT_MAX = 64;is not a constant expression in ISO C, so the three DP arrays (prev2/prev/curr) would become VLAs — a regression from compile-time sizing.- An anonymous
enum { NXT_CONF_VLDT_EDIT_MAX = 64 }would work mechanically, but Unit uses anonymous enums only for semantically related groups of named constants (e.g.NXT_UNIT_OK/ERROR/AGAIN/CANCELLEDinnxt_unit.h). A single-value enum has no precedent in the tree.
The macro is already TU-private (no header exposure, no NXT_EXPORT), so encapsulation is as tight as it can get for a C99 translation unit.
Generated by Claude Code
2 participants
Implements roadmap item D5 (
roadmap/unit-roadmap.md:141-144).Summary
location.path) so users can see where in a nested config the failure is.suggestion).error,detail,location.offset/line/column) and existing error-message wording are byte-for-byte unchanged.test/test_config_validation_errors.pywith 10 targeted cases (path threading, RFC 6901 escaping, suggestion thresholds, backward-compat guards).Plan
Full design + mermaid diagrams:
roadmap/plan-config-errors.md(committed in this PR).Test plan
./configure && make -jbuilds cleanlypytest test/test_config_validation_errors.py -vall greenpytest test/test_configuration.py -vstill green (backward-compat smoke)curl -X PUT --data-binary '{"listners":{}}' --unix-socket <sock> http://localhost/configreturns a body containinglocation.pathandsuggestion: "listeners"Draft while implementation lands.