Skip to content

Conversation

@thevilledev
Copy link
Contributor

@thevilledev thevilledev commented Nov 15, 2025

From #860 (comment)

Disallow negative offsets for forward jump opcodes in the VM. A crafted Program or fuzzed bytecode could pass negative arguments for OpJump and the conditional jump variants, causing unsafe control flow. Now these opcodes panic with a clear error when given a negative offset. Worth noting that the compiler never emits negative offsets, for example see in compiler.patchJump.

The test times out against master branch as it's basically an infinite loop.

Bench test showing not much of a difference:

goos: linux
goarch: amd64
pkg: github.com/expr-lang/expr
cpu: AMD EPYC 9575F 64-Core Processor
                            │   before    │              after-jump               │
                            │   sec/op    │    sec/op      vs base                │
_expr-32                      47.99n ± 3%    45.76n ±  0%   -4.64% (p=0.000 n=10)
_expr_eval-32                 4.173µ ± 1%    4.100µ ±  1%   -1.75% (p=0.000 n=10)
_expr_reuseVm-32              30.41n ± 5%    30.43n ±  2%        ~ (p=0.699 n=10)
_len-32                       29.98n ± 1%    29.92n ±  1%   -0.20% (p=0.050 n=10)
_filter-32                    34.79µ ± 1%    32.08µ ±  3%   -7.79% (p=0.000 n=10)
_filterLen-32                 29.33µ ± 4%    28.65µ ±  0%   -2.31% (p=0.000 n=10)
_filterFirst-32               318.6n ± 1%    298.8n ±  1%   -6.20% (p=0.000 n=10)
_filterLast-32                549.2n ± 1%    548.8n ±  1%        ~ (p=0.644 n=10)
_filterMap-32                 3.764µ ± 1%    3.704µ ±  1%   -1.59% (p=0.000 n=10)
_arrayIndex-32                51.59n ± 9%    46.25n ±  2%  -10.34% (p=0.002 n=10)
_envStruct-32                 33.23n ± 1%    32.01n ±  1%   -3.66% (p=0.000 n=10)
_envMap-32                    37.17n ± 1%    36.20n ±  0%   -2.60% (p=0.000 n=10)
_callFunc-32                  234.0n ± 1%    232.5n ±  1%   -0.66% (p=0.033 n=10)
_callMethod-32                248.3n ± 1%    248.7n ±  1%        ~ (p=0.839 n=10)
_callField-32                 49.24n ± 1%    48.90n ± 17%        ~ (p=0.516 n=10)
_callFast-32                  50.42n ± 5%    49.58n ±  0%        ~ (p=0.052 n=10)
_callConstExpr-32             19.52n ± 3%    21.53n ±  0%  +10.27% (p=0.000 n=10)
_largeStructAccess-32         94.00n ± 1%    95.96n ±  1%   +2.08% (p=0.002 n=10)
_largeNestedStructAccess-32   99.37n ± 1%   100.85n ±  1%   +1.49% (p=0.001 n=10)
_largeNestedArrayAccess-32    460.0µ ± 1%    469.1µ ±  1%   +1.98% (p=0.000 n=10)
_sort-32                      2.636µ ± 2%    2.719µ ±  1%   +3.13% (p=0.001 n=10)
_sortBy-32                    6.575µ ± 1%    6.891µ ±  0%   +4.81% (p=0.000 n=10)
_groupBy-32                   7.182µ ± 1%    7.745µ ±  0%   +7.84% (p=0.000 n=10)
_reduce-32                    3.326µ ± 2%    3.504µ ±  1%   +5.37% (p=0.000 n=10)
geomean                       491.3n         489.4n         -0.40%

Disallow negative offsets for forward jump opcodes in the VM.
The compiler only ever emits non-negative offsets, but a crafted
Program or fuzzed bytecode could pass negative arguments for
OpJump and the conditional jump variants, causing unsafe control
flow. Now these opcodes panic with a clear error when given a
negative offset.

Signed-off-by: Ville Vesilehto <[email protected]>
@antonmedv
Copy link
Member

Those benchmarks unfortunately inconclusive

@thevilledev
Copy link
Contributor Author

thevilledev commented Nov 15, 2025

I also tried it with pathological cases, like deep nested conditionals:

exprStr := `
  ((a > 0 && b > 0) || (a > 1 && b > 1) || (a > 2 && b > 2) || (a > 3 && b > 3) || (a > 4 && b > 4)) &&
  ((c < 10 || c < 9 || c < 8 || c < 7 || c < 6 || c < 5)) &&
  (a == 1 ? (b == 2 ? c == 3 : b == 2 || c == 3) : (a == 1 || b == 2 || c == 3))
`

But of course the compiler is guaranteed to push non-negative jumps. I disassembled the vm.Run to see what it does under the hood. On ARM64 the if arg < 0 branch translates to:

  vm.go:179             0x1001f17e4             b7fa6fa2                TBNZ $63, R2, 4989(PC)

Meaning "test bit and branch if non-zero". Branch prediction with compiler only emitting non-negative numbers makes this very lightweight of a check. So IMO the impact of this change is almost a no-op.

On x86-64:

  vm.go:179             0x629720                4885ff                          TESTQ DI, DI
  vm.go:179             0x629723                0f8c7c580000                    JL 0x62efa5

Checks the sign of arg first, which compiler guarantees to be non-negative. The "jump if less" that follows is a conditional branch that I'd guess is ~never taken due to branch prediction. The sign check is one ALU op.

Happy to hear other ideas/concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants