fix: bound array insert/fill/slice indices and reject diverging security symbol rebinds#105
Merged
Merged
Conversation
array.insert, the three-argument array.fill overload, and array.slice
lowered straight to std::vector::insert / std::fill / vector-range
construction with a raw (int) cast on the Pine index. Any index outside
the array made the emitted C++ form an iterator outside [begin, end] —
undefined behaviour. Observed effects on a built engine: array.insert
with a negative index wrote to the wrong slot and returned a wrong value
with no diagnostic, and an out-of-range slice/fill crashed the process
(SIGBUS) instead of surfacing Pine's runtime error.
The rest of the indexed array family (get/set/remove/first/last/pop/
shift/percentrank) already converts an out-of-range index into
pine_runtime_error. Route these three through the same prelude.
The prelude gains two parameters because these three do not share the
element-access bound:
* allow_size — an INSERTION point is valid at index == size
(array.insert(id, size, v) appends), and the half-open
[index_from, index_to) ranges of fill/slice legally end at size.
An element access is not. Using the element bound here would reject
valid Pine; array.insert(a, array.binary_search_rightmost(a, v), v)
is exactly the shape that appends.
* name — renames the emitted locals so both endpoints of a range can
be checked in one C++ scope. name="index" reproduces the previous
identifiers byte-for-byte, so the existing lowerings are unchanged.
Negative indices follow the Pine v6 reference: insert normalises them
(it is listed with get/set/remove as a negative-indexing function),
fill/slice reject them (they are not), matching percentrank's stance.
array.slice has two emission sites — the typed method lane in
codegen/types.py and the ARRAY_METHODS fallback — so the checked helper
takes the result type from the caller and both lanes use it. Guarding
only one would leave the live path unchecked.
CHECKED_ARRAY_METHOD_KWARGS gains insert/fill/slice so their keyword
call forms merge into signature order instead of dropping every kwarg.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two existing tests pinned the literal text of the unchecked array.slice emission (``std::vector<T>(<receiver>.begin()+(int)(...)``). The bounds guard binds the receiver to a lambda parameter, so the iterators now come from ``__pf_array`` and the receiver appears once at the call site. Both assertions keep their original intent — the slice preserves the receiver's ELEMENT type, and a temporary receiver is evaluated exactly once — expressed against the new emission instead of the old text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
register_security_eval carries no symbol (emit_top.py emits sec_id, tf and flags only), so every accepted request.security runs on the CHART feed. _scalar_defs was populated only by setdefault in _visit_VarDecl, with no Assignment visitor, so a declaration was treated as pinning the value forever: var sym = syminfo.tickerid followed by sym := "EXCH:OTHER" transpiled clean and backtested chart data under the other symbol's name — a divergence invisible in a trade diff. Record every := (and compound-op) rebind and require each to resolve to the chart symbol as well as the declaration. A compound op records its right-hand operand, so a string-built symbol is rejected by the same rule. Rebinds are collected in a whole-AST pre-pass as well as in the new _visit_Assignment, because the visit walks statements in source order and a rebind placed AFTER the request.security call would otherwise never be seen by it. The pre-pass is strictly fail-closed: it can only add rejections. The sibling ternary-branch hole is deliberately NOT closed here — it is not fail-closed on the current corpus and is tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Two independent fail-closed guards
1. Array index bounds —
array.insert, 3-argarray.fill,array.slicePine treats an array index as a checked value: an index outside the array raises
a runtime error that halts the script. The valid domain depends on the operation's
role, not one rule — element access is valid over
[0, size), an insertion pointis valid over
[0, size]because inserting atsizeappends, and a half-open[index_from, index_to)endpoint is valid over[0, size]becauseindex_toisexclusive.
insertadditionally inherits end-relative negative indexing (thereference lists it with
get/set/remove);fillandslicedo not.These three lowerings had erased that role distinction into an unchecked
(int)cast, so an invalid Pine index became C++ undefined behaviour instead of a
diagnosable Pine error. Demonstrated on a built engine, not theorised:
array.insert(a, -1, v)silently wrote the wrong slot and returned a wrong valuewith no diagnostic;
slice/fillSIGBUS'd the process.They now route through the existing checked-index machinery that already backs
get/set/remove/first/last/pop/shift/percentrank, with deterministicmessages (
Index 4 is out of bounds. Array size is 3,Index range 2..1 is invalid. Array size is 3).Note
array.slicehas two emission sites — the typed-method lane and theARRAY_METHODSfallback. Guarding only the fallback would have left the live pathunchecked, so both use the checked helper and element types are preserved exactly.
Two pre-existing assertions that pinned the literal text of the unchecked slice
emission are retargeted at the checked lowering, keeping their original intent.
array.insert(a, size, v)(append) is preserved and pinned by three passing tests,including one that links and runs a real binary — a naive
< sizebound wouldhave broken a live usage whose index is a
binary_search_rightmostresult.2.
request.securitysymbol —:=rebindIn Pine a
request.securitysymbol argument is a value, and the data loaded iswhatever it evaluates to at runtime. This engine can only load the chart symbol, so
the checker must prove the argument is the chart symbol under every reachable
evaluation. It read a variable's declaration while ignoring its
:=rebinds, sovar sym = syminfo.tickeridfollowed bysym := "EXCH:OTHER"passed — and since thesecurity registration carries no symbol, the result silently resolved to the chart
feed, producing plausible trades attributed to a symbol never loaded.
Rebinds (including compound-assignment forms) are now recorded and a divergent rebind
is rejected.
Deliberately NOT included
The sibling hole — accepting a ternary symbol when either branch is the chart
symbol — is held back on a separate branch. Tightening it would reject a
currently-correct strategy whose declared input configuration provably selects the
chart-symbol branch, deleting a 100 %-matching row from the graded set. That is a
worse immediate error than an unexercised hole.
Refining it to "reject only when the divergent branch is selectable at the compiled
configuration" was investigated and is unsound: the checker can see input
defaults but not overrides, which resolve at runtime, so folding defaults would leave
the hole reachable while looking fixed. Threading configuration into the transpiler
would change its contract from
source -> C++to(source, config) -> C++and breakthe determinism every byte-identity gate depends on.
The sound fix is to thread the resolved symbol into security registration and reject
at runtime; that touches a public signature and belongs in its own change.
Gates
runtime probes (wrong-slot write; SIGBUS). All 4 rebind checks failed. Positive
controls passed before and after.
other movement. (Requires the Eigen include; without it 801 tests silently skip.)
base vs patched:
success_set_changed: 0,failure_message_changed: 0, andexactly 3 sources' emitted C++ changed — all
array.insert/array.sliceusers, diffs purely the guard wrapper replacing raw iterator arithmetic, all still
compiling clean.
Residuals recorded in
docs/codegen-coverage-gaps.mdarray.slicealiasing: the reference says a slice shares storage with its source,while both lowerings deep-copy. Needs a view type; the doc wording ("an object from
the slice") does not settle element-for-element aliasing for arrays of primitives,
so that must be pinned before designing it.
fill/sliceendpoints are rejected — both fail-closed choices with no evidence pinning
TradingView's behaviour.
existing declaration table; a divergent rebind of a local could disqualify an
unrelated global. Fail-closed, and the 788-source differential shows it costs
nothing today.