Skip to content

fix(gc): implement better way to handle root overload in inc_non_root_count#4936

Merged
jedel1043 merged 2 commits intoboa-dev:mainfrom
shruti2522:fix-non-root-count
Mar 13, 2026
Merged

fix(gc): implement better way to handle root overload in inc_non_root_count#4936
jedel1043 merged 2 commits intoboa-dev:mainfrom
shruti2522:fix-non-root-count

Conversation

@shruti2522
Copy link
Contributor

It changes the following:

saturating at NON_ROOTS_MAX pins the counter instead of panicking. pinned counter returns true for is_rooted so the object is kept alive rather than freed, a leak is safer than a UAF anyways. The limit ~2^31 heap references to one object is unreachable in practice

@shruti2522 shruti2522 requested a review from a team as a code owner March 7, 2026 22:04
@github-actions
Copy link

github-actions bot commented Mar 7, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,932 49,932 0
Ignored 2,210 2,210 0
Failed 821 821 0
Panics 0 0 0
Conformance 94.28% 94.28% 0.00%

Tested main commit: 5ee4764a3f2b6c8d82a9cc7e9fd5206cfddde65d
Tested PR commit: 0035829c40cc3cfe7f3dc1f426db0e29074de7bd
Compare commits: 5ee4764...0035829

@codecov
Copy link

codecov bot commented Mar 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.06%. Comparing base (6ddc2b4) to head (0035829).
⚠️ Report is 841 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4936       +/-   ##
===========================================
+ Coverage   47.24%   59.06%   +11.82%     
===========================================
  Files         476      563       +87     
  Lines       46892    62365    +15473     
===========================================
+ Hits        22154    36837    +14683     
- Misses      24738    25528      +790     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jedel1043
Copy link
Member

Well that would make it so that we underflow after all references get dropped. right? Then we're patching one panic with another one

@shruti2522
Copy link
Contributor Author

Well that would make it so that we underflow after all references get dropped. right? Then we're patching one panic with another one

oh yes, missed that 😅 let me look into this more carefully

@shruti2522 shruti2522 force-pushed the fix-non-root-count branch from dd01454 to ff6e1bf Compare March 8, 2026 14:41
@shruti2522 shruti2522 changed the title fix(gc): saturate non_root_count at NON_ROOTS_MAX instead of panicking fix(gc): implement better way to handle root overload in inc_non_root_count Mar 8, 2026
@shruti2522
Copy link
Contributor Author

I have capped non_root_count so it can never exceed ref_count. This guarantees is_rooted will never return false just because the counter overflowed, so we won't accidentally free a live object and cause a UAF

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Thank you for fixing the TODO

@jedel1043 jedel1043 enabled auto-merge March 13, 2026 06:22
@jedel1043 jedel1043 added technical debt gc Issue related to garbage collection labels Mar 13, 2026
@jedel1043
Copy link
Member

Ping me after you address the CI errors so that I can merge

auto-merge was automatically disabled March 13, 2026 10:33

Head branch was pushed to by a user without write access

@shruti2522
Copy link
Contributor Author

Ping me after you address the CI errors so that I can merge

@jedel1043 fixed the CI, ready for merge

@jedel1043 jedel1043 added this pull request to the merge queue Mar 13, 2026
Merged via the queue into boa-dev:main with commit aa9a070 Mar 13, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gc Issue related to garbage collection technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants