Skip to content

Pre-check json.dumps indent string size against resource limits#436

Open
dsp-ant wants to merge 1 commit into
pydantic:mainfrom
dsp-ant:fix/json-dumps-indent-resource-check
Open

Pre-check json.dumps indent string size against resource limits#436
dsp-ant wants to merge 1 commit into
pydantic:mainfrom
dsp-ant:fix/json-dumps-indent-resource-check

Conversation

@dsp-ant

@dsp-ant dsp-ant commented May 14, 2026

Copy link
Copy Markdown
Contributor

The integer indent= argument was converted directly into a repeated space string on the native heap with no resource pre-check, so a large value would allocate well past the configured memory budget. Validate the requested count against the resource tracker before materializing the indent string, raising MemoryError instead. Other .repeat() call sites already carry the same guard.


Summary by cubic

Pre-checks json.dumps(indent=...) against resource limits to prevent unbounded indent string allocation. Large values now raise MemoryError before allocating, matching guards used at other .repeat() sites.

  • Bug Fixes
    • Validate indent size via check_repeat_size before building the space string.
    • Added tests for rejecting huge indents and allowing small indents within limits.

Written for commit 582ea1b. Summary will update on new commits.

The integer indent= argument was converted directly into a repeated
space string on the native heap with no resource pre-check, so a
large value would allocate well past the configured memory budget.
Validate the requested count against the resource tracker before
materializing the indent string, raising MemoryError instead. Other
.repeat() call sites already carry the same guard.
@codspeed-hq

codspeed-hq Bot commented May 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 17 untouched benchmarks
⏩ 15 skipped benchmarks1


Comparing dsp-ant:fix/json-dumps-indent-resource-check (582ea1b) with main (ac5b383)

Open in CodSpeed

Footnotes

  1. 15 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 2 files

@codecov

codecov Bot commented May 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty/src/modules/json/dump.rs 62.50% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@davidhewitt davidhewitt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem like the right defence to me; the indent can be used multiple times over when dumping so a single check doesn't appear to make sense.

I have been playing around with a StringBuilder type which checks against memory limits as the string is formed, I think the correct defence for the JSON module in general is probably to switch to that construction rather than apply a defence at this point?

@davidhewitt

Copy link
Copy Markdown
Collaborator

I pushed the StringBuilder experiment to #448, I will see how it looks here.

@samuelcolvin

Copy link
Copy Markdown
Member

@davidhewitt is this now fixed?

@davidhewitt

Copy link
Copy Markdown
Collaborator

I have a branch (not pushed yet) which uses StringBuilder in json.dumps. The challenge is that the ResourceTracker has to be slightly decoupled from the Heap so that the builder can have a reference to the tracker while the VM is busy traversing heap values. Most likely will finish this week after next.

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