Fix Minja OOB & Parser Recursion Vulnerabilities - #1070
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an out-of-bounds read in the embedded Minja template engine when applying Python-style string slicing with step != 1 and out-of-range indices, improving safety for chat-template rendering in onnxruntime-extensions.
Changes:
- Clamp string slice
start/endindices inminja::SubscriptExpr::do_evaluatebefore the stepped indexing loop. - Add a regression test covering previously OOB-inducing slice patterns in chat template evaluation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| shared/api/minja.hpp | Clamps string slice endpoints to prevent OOB reads when stepping through std::string via indexing. |
| test/pp_api_test/test_tokenizer_chat.cc | Adds a regression test that exercises out-of-range slice indices with non-unit step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kunal-vaishnavi
approved these changes
Jun 8, 2026
added 2 commits
June 8, 2026 16:09
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.
Fix OOB read in minja string slice + stack overflow in recursive parser
Summary
Two security fixes for the minja template engine used by
OrtxApplyChatTemplate:SubscriptExpr::do_evaluatestring slice withstep != 1and out-of-range indices could read past the string buffer, leaking adjacent memory into rendered output.parseExpressionchain) had no depth limit, allowing a crafted template as small as 8 bytes to exhaust the call stack.Problem 1: String slice OOB read
When a chat template applies a slice with
step != 1to a string (e.g.'abc'[0:65536:2]),SubscriptExpr::do_evaluateuses a loop that indexes the string viastd::string::operator[]. Thewrap()helper only adjusts negative indices by addinglen, but never clamps indices to[0, len). This means:|i| > lenremain negative after adjustmentThe
step == 1fast-path safely usessubstr, but anystep >= 2or negative step bypasses it and hits the uncheckeds[i]loop — reading out-of-bounds memory.Problem 2: Parser recursion stack overflow
The
minja::Parserclass implements expression parsing as a chain of ~12 mutually recursive functions (parseExpression→parseLogicalOr→ ... →parseValueExpression→parseDictionary→parseExpression) with no recursion depth limit. A crafted template containing nested delimiters causes unbounded recursion, exhausting the stack and crashing the process.Fix
Slice OOB — Added Python-style slice endpoint clamping in the string branch of
SubscriptExpr::do_evaluate:Stack overflow — Added a recursion depth counter with RAII guard to the
Parserclass, checked at the entry ofparseExpression:Files changed
shared/api/minja.hpp— clamp slice endpoints before the indexing loop; add recursion depth guard to parsertest/pp_api_test/test_tokenizer_chat.cc— new testsMinjaStringSliceOOBClampedandMinjaParserRecursionDepthLimitTesting
New tests:
MinjaStringSliceOOBClamped— verifies three cases that previously caused OOB reads:'abc'[0:65536:2]→"ac"(positive step, end far beyond length)'abc'[100:0:-1]→"cb"(negative step, start beyond length)'abc'[-100:2:1]→"ab"(negative index that remains negative after wrap)MinjaParserRecursionDepthLimit— verifies that a deeply nested template (200 levels of{) returns an error instead of crashing with a stack overflow.All
pp_api_testchat template tests pass.