Skip to content
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

fix: Prevent stack overflow in recursive const types #16915

Merged
merged 2 commits into from Mar 24, 2024
Merged

Conversation

6d7a
Copy link
Contributor

@6d7a 6d7a commented Mar 21, 2024

In the evaluation of const values of recursive types certain declarations could cause an endless call-loop within the interpreter (hir-ty’s create_memory_map), which would lead to a stack overflow.
This commit adds a check that prevents values that contain an address in their value (such as TyKind::Ref) from being allocated at the address they contain.
The commit also adds a test for this edge case.

In the evaluation of const values of recursive types
certain declarations could cause an endless call-loop
within the interpreter (hir-ty’s create_memory_map),
which would lead to a stack overflow.
This commit adds a check that prevents values that contain
an address in their value (such as TyKind::Ref) from being
allocated at the address they contain.
The commit also adds a test for this edge case.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2024
@Veykril Veykril requested a review from HKalbasi March 22, 2024 08:59
@HKalbasi
Copy link
Member

@6d7a Thanks for the PR! I didn't understand how your patch addresses the problem. If I understand correctly, the problem is that TAG_TREE and VARIANT_TAG_TREE will get same address which will break things (here, and probably in other places too) but I'm not sure how this PR prevents that and how well it generalize to other cases.

In addition to that bug which we need to fix, infinite constants might exist, so we might need to add a depth limit or something like that to the create_memory_map function.

@6d7a
Copy link
Contributor Author

6d7a commented Mar 24, 2024

@HKalbasi thank you for your review. My reasoning in case of the fix was to prevent that reference values point to the location that they themselves reside in. However I can see that this is more of a hotfix for my particular case instead of a sustainable solution. Too much wishful thinking on my part.
I have removed the fix and added a stack depth check by propagating the Evaluator's stack depth limit along the call chain of create_memory_map. That prevents a terminal stack overflow, which makes rust-analyzer impossible to use in certain projects, and instead returns a soft internal error. I adapted the test accordingly.
I'd be interested in finding a way to better handle recursive const cases, though, but I guess that's for another PR.

@HKalbasi
Copy link
Member

Thanks!
@bors r+

I'd be interested in finding a way to better handle recursive const cases, though, but I guess that's for another PR.

AFAIK there is no way to create infinite constants in stable Rust, so this is good for now.

Though we should find out why it is allocating the same address for different constants, as it can cause problems even without recursive types.

@bors
Copy link
Collaborator

bors commented Mar 24, 2024

📌 Commit 142ef76 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 24, 2024

⌛ Testing commit 142ef76 with merge 3dfd4c1...

@bors
Copy link
Collaborator

bors commented Mar 24, 2024

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 3dfd4c1 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants