Repository navigation
fix(core): bound arithmetic and sequence sizes in safe_expr - #947
elijahbenizzy wants to merge 5 commits into
Conversation
Condition.safe_expr() allowed a handful of operators whose result size is not bounded by the size of the expression: int ** int, sequence * n, sequence + sequence, and printf-style % on strings. Bound them: - at most 500 AST nodes per expression (checked before the recursive validator runs) - int ** int: |exponent| <= 64 and base <= 64 bits; non-numeric operands rejected; float ** left to Python (bounded by float range) - sequence * n: n <= 10_000 and result <= 1_000_000 elements; same result cap for sequence + sequence - % is no longer applied to strings - general backstop: no arithmetic step or int literal wider than 4096 bits Literal-only subexpressions (e.g. 10 ** 10 ** 10) are checked at safe_expr() call time; state-dependent values raise ValueError from run(). Docstring updated and tests added for each bound.
The node-count bound runs on the parsed tree, but on Python 3.11 the parser
itself gives up on a 3000-term chain with RecursionError before that check
is reached. Two complementary bounds, both applied before/around ast.parse:
- the expression string may be at most 10_000 characters
(_SAFE_EXPR_MAX_SOURCE_CHARS)
- RecursionError / MemoryError raised by ast.parse are re-raised as
ValueError('safe_expr: expression is too deeply nested'); SyntaxError
is unchanged
Tests: the deep-chain node-count case now uses 600 terms (parses on every
supported Python), the 3000-term chain is asserted to raise ValueError
regardless of which bound trips first, and a 20_000-character expression
is rejected on length. Docstring updated.
sangkyoonnam
left a comment
sangkyoonnam
left a comment
There was a problem hiding this comment.
The cases from the description, 10**10**10 and "a" * 10000000000, are now rejected when safe_expr() builds the condition, and the 61 safe_expr tests pass. The docstring spells out the new rules. Two of their effects might be worth a second look.
The ±64 exponent rule rejects 2**100 > 0 and 10**100 > 0 with "exponent out of range", while 2.0**100 passes. For int ** int with a non-negative exponent, an estimate like base.bit_length() * exp <= 4096 would still reject 9**9**9 and let those through, alongside the existing operand-type and 4096-bit result checks.
'%d items' % n and n ** 2 with a NumPy integer or a Decimal from state worked on main and now raise when the condition runs. Intended? If so, a short upgrade note in the description would help anyone whose rules hit it.
…d types in safe_expr Follow-ups to the safe_expr bounds, to bound resource use more tightly: - Track the logical size (elements a full walk would visit, counting shared substructure each time it is reached) of every container the interpreter builds, and cap it at 1,000,000 for *, + and container displays. Nested repeats such as [[[[0]*1000]*1000]*1000]*1000 passed the per-step length check but have 10**12 leaves; they are now rejected before anything is built. The same cap is applied before an operation walks a container (==/!= on equal lengths, ordering comparisons, in / not in, and builtins other than len and bool), measuring state values once with a bounded walk. - +, -, *, % and ** accept only int (anything with __index__, coerced via operator.index so NumPy integers are bounded like plain ints), float, str, bytes, list and tuple. Other operand types (deque, array.array, Decimal, arrays, custom objects) raise ValueError instead of sidestepping the repeat and width bounds. - The construction-time literal check only evaluates subtrees made of numeric / string constants, so safe_expr() no longer builds container displays; arithmetic and type errors are deferred to run(), and memory or recursion exhaustion is reported as ValueError rather than swallowed. - int ** int is bounded on the estimated result width, bit_length(base) * exponent <= 4096 bits (negative exponents: magnitude <= 4096), replacing the fixed +/-64 exponent cap so 2**100 and 10**100 work while 9**9**9 is still rejected.
|
@skrawcz ready for review. @sangkyoonnam all three paths you found are closed: logical (deep) size is tracked and checked before any walk, operand types are whitelisted with |
…le dict fallback - Docstring: bare names resolve against state first and fall back to the allowlisted builtins; a call target always binds the allowlisted builtin even if state has a key of that name, intentionally unlike expr(). - _eval_Dict: the validator now rejects dictionary unpacking (a None key) at construction, so the None-key fallback in evaluation was unreachable.
This branch has not been deployed
2 participants
Condition.safe_expr()is documented for expressions from less-trusted sources, but its interpreter placed no bound on arithmetic.10**10**10or"a" * 10000000000passed validation and then ran until the process was killed.This change adds explicit bounds, documented in the docstring: expression size (500 nodes), exponent and base width for
**, repeat count and result length for sequence*/+, and a 4096-bit backstop on every integer result. Literal-only subtrees are checked atsafe_expr()construction so the obvious cases fail early; everything else fails atrun()with a plainValueError.%with a string left operand is rejected for the same reason.No existing test changed; 12 added.
Update after review. Sequence bounds are now on logical size (nested repeats of a shared object count every leaf), enforced before allocation and before any operation that walks a sequence; cap 1,000,000. Arithmetic operands are whitelisted: anything with
__index__is coerced (so NumPy ints work and are bounded),floatpasses, sequences must bestr/bytes/list/tuple;Decimal,Fraction,deque,array.array, arrays, dates and sets raise a clearValueErrorinstead of running unbounded. The construction-time pre-check only evaluates scalar literals and convertsMemoryError/RecursionErrortoValueError.int ** intis bounded by estimated result width (bit_length(base) * exp <= 4096) rather than a fixed exponent, so2**100 > 0works and9**9**9does not.Upgrade notes:
[[0]*1000]*1000is now over the cap; state containers over one million logical elements raise in walk operations (len,bool,is, and==on different lengths still work). Known worst cases at the cap, bounded but not cheap:sum(list_of_lists, [])is quadratic (~3 s), andstr()of 100,000 4096-bit ints is ~120 MB.