-
Notifications
You must be signed in to change notification settings - Fork 717
fix: soundness bug in {Expr, Level}.data #8554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Conversation
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
Rob23oba
reviewed
May 30, 2025
Rob23oba
reviewed
May 30, 2025
Collaborator
|
Mathlib CI status (docs):
|
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 31, 2025
This PR fixes an adversarial soundness attack described in #8554. The attack exploits the fact that `assert!` no longer aborts execution, and that users can redirect error messages. Another PR will implement the same fix for `Expr.Data`.
Member
|
I'll close this as it is covered by the PR above |
Collaborator
Author
|
If you are referring to #8559, that only covers one half of this issue, and it says "Another PR will implement the same fix for |
Contributor
|
Note: #8560 also got merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
changelog-language
Language features and metaprograms
toolchain-available
A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
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 fixes an issue with handling of extremely large or deep expressions in the kernel which could lead to unsound behavior in some scenarios.
The problem
Every
ExprandLevelcomes with a small computed field which keeps track of some aggregate data. Because this is a fixed size field, only small or approximate information can be stored, and unlike e.g. the maximum bound on array sizes, this is visible to lean code and the bit tricks are implemented in lean itself. Both types are however using this field for a nominally infinite piece of data:Leveltracks the depth of the expression, andExprtracks the largest loose bvar index. Computed fields are pure functions, which have no error pathway, so this leads to a problem - how to handle overflow?Currently, this is handled using
assert!and/orpanic!. Butpanic!is not a good substitute for error handling, because a panic does not abort execution, and lean continues with the default value for the type. In this case, that means inserting a0for the wholeLevel.Data/Expr.Datafield, which is the worst possible answer: it means that the expression appears to have no loose bvars, no mvars, no fvars, even if it actually does, meaning that all of the functionsExpr.hasFVar,Expr.hasExprMVar,Expr.hasLevelMVar,Expr.hasMVar,Expr.hasLevelParam,Expr.hasLooseBVarsfail to be conservative, which leads to unsoundness (a test is included). See also the discussion on zulip.The solution
Changing the computed data field to be an
Optionor otherwise contain error handling would likely be too expensive, and result in significant API changes. Instead, we simply make the data field give conservative answers when an overflow occurs, and avoid contaminating unrelated fields in the bitfield.Level, we can simply saturate the depth at the maximum value, as is already done insideExpr. This field isn't even used for anything as far as I can tell, and there should be no added checks since the assertion was already present.Expr, themkDatafunction is performance critical, so I tried to avoid any unnecessary branches or bounds checks. We can again saturate when the bound variable index overflows, but because we subtract 1 fromlooseBVarRangewhen constructing a binder, we have to take care to not reduce an overflowed value. This ensures conservativity. (One slight change here is that the maximum bvar number has to be decreased by 1, from2^20-1to2^20-2to leave space for the overflow value.)In order to compensate for the removed asserts, the handling of this field is now mediated by the
BVarRangetype, which is just a wrapper forUInt32. I believe that the fact that this is a bounded type will actually improve performance here; the original version of the code was converting to and fromNatin order to do the arithmetic.