Skip to content

fix: prevent silent integer overflows in rate limiter and expression …#47

Open
Pama-Lee wants to merge 3 commits intomainfrom
claude/review-project-jmJWV
Open

fix: prevent silent integer overflows in rate limiter and expression …#47
Pama-Lee wants to merge 3 commits intomainfrom
claude/review-project-jmJWV

Conversation

@Pama-Lee
Copy link
Copy Markdown
Collaborator

  • rate_limiter: clamp tokens_to_add to capacity before u32 cast to prevent truncation when server uptime is very long; use saturating_add before .min() to prevent u32 overflow in token accumulation
  • compiler: assert pool sizes (constants/fields/functions) stay within u8 limit (< 256) instead of silently truncating indices, which caused wrong values to be loaded at runtime; same for register count and function argument count

…compiler

- rate_limiter: clamp tokens_to_add to capacity before u32 cast to prevent
  truncation when server uptime is very long; use saturating_add before .min()
  to prevent u32 overflow in token accumulation
- compiler: assert pool sizes (constants/fields/functions) stay within u8 limit
  (< 256) instead of silently truncating indices, which caused wrong values to
  be loaded at runtime; same for register count and function argument count

https://claude.ai/code/session_01UMp8kxnrNeDbpcr1M7W1oN
@Pama-Lee Pama-Lee requested a review from Andero-Wang March 14, 2026 16:12
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ordo_docs Ready Ready Preview, Comment Mar 17, 2026 1:19am

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d6770dd987

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/ordo-core/src/expr/compiler.rs Outdated
Comment on lines +50 to +52
self.next_reg = self.next_reg.checked_add(1).expect(
"register limit exceeded: expression requires more than 256 registers",
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow allocation of register 255 before overflow check

alloc_reg now panics when next_reg == 255 because checked_add(1) is evaluated before returning, so register index 255 is never usable. That is an off-by-one regression: the VM uses 8-bit register operands and a 256-slot register file ([Value; 256] in vm.rs), and this same commit still treats 255 function arguments as valid (args.len() < 256). Expressions that require the highest register (e.g. wide function calls) now panic even though they are representable.

Useful? React with 👍 / 👎.

Expands the built-in function library from ~30 to ~68 functions:

String: split, join, replace, trim_start, trim_end, index_of, repeat, pad_left
Regex:  regex_match, regex_find, regex_replace, regex_split
        (thread-local LRU cache for compiled patterns, capacity 64)
Array:  sort, sort_desc, uniq, flatten, concat, slice, reverse, contains_any
Sets:   set_union, set_intersection, set_difference, is_subset
        (operate on Arrays, aligning with OPA set semantics)
Object: keys, values, merge, has_key, get_or
Encode: base64_encode, base64_decode, json_parse, json_stringify
Math:   pow, sqrt, log
Type:   is_bool, is_object

base64 moved from optional (signature feature) to unconditional dependency.
All 136 tests pass (8 new test groups added).

https://claude.ai/code/session_01UMp8kxnrNeDbpcr1M7W1oN
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.

3 participants