TooManyLiveRegs check: fix break that was broken by allocator change.#212
Merged
cfallin merged 1 commit intobytecodealliance:mainfrom Apr 1, 2025
Conversation
In bytecodealliance#196, various `break`s and early returns` out of a main allocator loop were changed to pass through a point that returned scratch storage to a context for later reuse. Unfortunately, this refactor introduced a bug: a `break` became a `break 'outer`, and the latter meant that we were skipping some checks. Fortunately this only happens in a code-path where the given allocation problem has too many constraints, and it means that we get a panic rather than a `TooManyLiveRegs` error cleanly returned, so this is not a security issue. However, it has me a little paranoid and we should carefully re-audit bytecodealliance#196's changes when we get a chance. (Discovered while making changes to ABI code that caused too many defs constrained to regs, resulting in an un-allocatable input; not reachable from mainline Cranelift.)
fitzgen
approved these changes
Apr 1, 2025
Member
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Do you know why the checker didn't catch this during fuzzing?
Member
Author
|
That's a great question; I suspect it has to do with our fuzzing function generator which adds up to four operands to any instruction and up to five clobbers. That would be sufficient to hit this if we had a smaller register environment but we have 32 registers per class. All of this provides sufficient coverage for everything but calls; and I suspect we haven't had any hits from higher-level fuzzing because the use-cases we generate were shaped in a particular way (only defs are fixed defs, per the ABI, so everything fits). I'll poke at this when I get some more time. |
Merged
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.
In #196, various
breaks and early returns` out of a main allocator loop were changed to pass through a point that returned scratch storage to a context for later reuse.Unfortunately, this refactor introduced a bug: a
breakbecame abreak 'outer, and the latter meant that we were skipping some checks.Fortunately this only happens in a code-path where the given allocation problem has too many constraints, and it means that we get a panic rather than a
TooManyLiveRegserror cleanly returned, so this is not a security issue. However, it has me a little paranoid and we should carefully re-audit #196's changes when we get a chance.(Discovered while making changes to ABI code that caused too many defs constrained to regs, resulting in an un-allocatable input; not reachable from mainline Cranelift.)
cc @jakubDoka