fix(ocr): preserve nested coordinate structure in extract_text_blocks - #5349
Open
Ricardo-M-L wants to merge 3 commits into
Open
fix(ocr): preserve nested coordinate structure in extract_text_blocks#5349Ricardo-M-L wants to merge 3 commits into
Ricardo-M-L wants to merge 3 commits into
Conversation
…nate parsing
`xinference/model/image/ocr/deepseek_ocr.py` had two `eval()` calls in
the coordinate-extraction helpers — `extract_coordinates_and_label`
(line 231) and `extract_text_blocks` (line 399). Both `eval()` their
input directly:
```python
cor_list = eval(ref_text[2]) # ref_text from OCR output
coords = eval(f"[{coords_str}]") # coords_str from OCR output
```
The argument to each `eval()` is a substring extracted from the OCR
model's text output. Because OCR output is LLM-generated and
attacker-influenceable through the source document or prompt, an OCR
response of the form
```
<|ref|>x<|/ref|><|det|>[[__import__('os').system('id')]]<|/det|>
```
would have its payload **executed by the host process** when the result
is parsed for downstream rendering / block extraction.
Replace both calls with `ast.literal_eval`, which only accepts Python
literal structures (lists, tuples, numbers, strings, …) and refuses
calls, attribute access, and imports. The expected coordinate strings
are list literals like `[[10, 20, 30, 40]]`, which `literal_eval`
parses identically to `eval`, so behavior on legitimate input is
unchanged. Exception handling is tightened to the specific
`ValueError` / `SyntaxError` that `literal_eval` raises (plus
`IndexError`/`TypeError` in the first helper because it indexes
`ref_text[1]`/`ref_text[2]`).
This is the same fix shape as the merged tool-parser PR xorbitsai#4786 — that
one covered `xinference/model/llm/`; this PR covers the image OCR
path that xorbitsai#4786 didn't reach.
Adds `xinference/model/image/ocr/tests/test_deepseek_ocr_safe_eval.py`
with cases that pin the security guarantee:
- Legitimate single and multi coordinate lists still parse.
- A payload of `__import__('pathlib').Path(...).write_text(...)` is
rejected with no side effect (verified by checking a tmp_path
sentinel file is never created).
- Attribute-walking payloads (`().__class__.__bases__[0]...`) are
rejected.
- Malformed truncated input does not crash callers.
- A malicious block followed by a valid block in the same OCR
response still allows the valid block to surface.
The pre-existing regex `\[\[(.*?)\]\]` stripped the outer `[[...]]`,
leaving `coords_str` as e.g. `1, 2, 3, 4`. After ast.literal_eval the
result was a *flat* list `[1, 2, 3, 4]`, which:
1. Disagrees with the sibling helper extract_coordinates_and_label,
which returns a nested list `[[1, 2, 3, 4]]`.
2. Breaks the immediately-following `bbox = coords[0] if
len(coords) == 1 else coords` line — that pattern only makes
sense when `coords` is a list-of-lists (length-1 means one bbox,
length>1 means multiple). With a flat list, `coords[0]` returns
a single integer.
3. Causes the new safe_eval tests in this file to fail.
Fix the regex to capture the outer brackets and use literal_eval
directly on `coords_str`. Verified locally that:
- Single block: coords == [[1, 2, 3, 4]], bbox == [1, 2, 3, 4]
- Multiple blocks: each produces its own [[...]]
- Code-injection payload still rejected by literal_eval (ValueError)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces unsafe eval calls with ast.literal_eval in deepseek_ocr.py to mitigate remote code execution (RCE) vulnerabilities from LLM-generated OCR outputs, and introduces comprehensive unit tests to verify this behavior. The reviewer recommends also catching MemoryError and RecursionError during parsing to protect against potential denial-of-service (DoS) attacks from maliciously crafted inputs.
| # Python literal structures (lists/tuples/numbers) and refuses calls, | ||
| # imports, and attribute access. | ||
| cor_list = ast.literal_eval(ref_text[2]) | ||
| except (ValueError, SyntaxError, IndexError, TypeError) as e: |
Contributor
There was a problem hiding this comment.
ast.literal_eval can also raise MemoryError and RecursionError on maliciously crafted input, which could lead to a denial of service. It would be more robust to catch these exceptions as well to prevent the process from crashing.
Suggested change
| except (ValueError, SyntaxError, IndexError, TypeError) as e: | |
| except (ValueError, SyntaxError, IndexError, TypeError, MemoryError, RecursionError) as e: |
| } | ||
| ) | ||
| except Exception: | ||
| except (ValueError, SyntaxError): |
Contributor
There was a problem hiding this comment.
Similar to the other ast.literal_eval call, this one could also raise MemoryError or RecursionError on maliciously crafted input, leading to a denial of service. Since this is inside a loop, an unhandled exception would stop the processing of subsequent text blocks. Catching these exceptions would make the parsing more robust.
Suggested change
| except (ValueError, SyntaxError): | |
| except (ValueError, SyntaxError, MemoryError, RecursionError): |
Ricardo-M-L
changed the title
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
2 participants
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.
This PR addresses: fix(ocr): preserve nested coordinate structure in extract_text_blocks