Skip to content

Fix buffer overflow/infinite loop from indent handling#709

Merged
bwoodsend merged 1 commit intomainfrom
big-indent
Mar 11, 2026
Merged

Fix buffer overflow/infinite loop from indent handling#709
bwoodsend merged 1 commit intomainfrom
big-indent

Conversation

@bwoodsend
Copy link
Copy Markdown
Collaborator

If indent * nest_depth is large enough to overflow an int, it causes the required output buffer size to be underestimated leading to a buffer overflow.

The offending arithmetic is upgraded to a ptrdiff_t and indent is artificially capped. An overflow is still technically possible given a high enough recursion depth but not without first consuming petabytes of RAM.

If indent is negative, it causes a size_t to underflow to some number a little bellow size_t max. If that underflow isn't accidentally rectified by a subsequent overflow (if -indent * (nest_depth + 1) > current_buffer_size) then the buffer up-sizer gets stuck in an infinite loop trying to find a power of two that fits in a size_t but is greater that size_t max / 2.

It's hard to tell if ujson.dumps(..., indent=-1) was ever an intentional feature but I don't feel comfortable breaking it in a security fix. For now, the dubious any negative indent -> pad colons but add no indentation or newlines behaviour is preserved but internally the indent is clipped to -1 and all subsequent indentation code paths are skipped over.

Fixes #700

If indent * nest depth is large enough to overflow an int, it causes the
required output buffer size to be underestimated leading to a buffer
overflow.

The offending arithmetic is upgraded to a ptrdiff_t and indent is
artificially capped. An overflow is still technically possible given a
high enough recursion depth but not without first consuming petabytes of
RAM.

If indent is negative, it causes a size_t to underflow to some number a
little bellow size_t max. If that underflow isn't accidentally rectified
by a subsequent overflow (if -indent * (nest_depth + 1) >
current_buffer_size) then the buffer up-sizer gets stuck in an infinite
loop trying to find a power of two that fits in a size_t but is greater
that size_t max / 2.

It's hard to tell if `ujson.dumps(..., indent=-1)` was ever an
intentional feature but I don't feel comfortable breaking it in a
security fix. For now, the dubious *any negative indent -> pad colons
but add no indentation or newlines* behaviour is preserved but
internally the indent is clipped to -1 and all subsequent indentation
code paths are skipped over.
@bwoodsend bwoodsend added the changelog: Fixed For any bug fixes label Mar 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.22%. Comparing base (a465ed7) to head (1860ed8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   91.08%   91.22%   +0.14%     
==========================================
  Files           7        7              
  Lines        1951     1971      +20     
==========================================
+ Hits         1777     1798      +21     
+ Misses        174      173       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bwoodsend bwoodsend requested a review from hugovk March 9, 2026 19:22
Copy link
Copy Markdown
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks, I think an error is better than silently capping.

@bwoodsend bwoodsend merged commit 486bd45 into main Mar 11, 2026
52 checks passed
@bwoodsend bwoodsend deleted the big-indent branch March 11, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: Fixed For any bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ujson.dumps crashes when indent is large

2 participants