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

Recursion limit silently ignored if it overflows a usize #67265

Closed
ecstatic-morse opened this issue Dec 12, 2019 · 2 comments · Fixed by #67272
Closed

Recursion limit silently ignored if it overflows a usize #67265

ecstatic-morse opened this issue Dec 12, 2019 · 2 comments · Fixed by #67272
Assignees
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Dec 12, 2019

The following code fails to compile since it exceeds the default recursion limit. Using 2056 as the recursion limit works as expected.

#![recursion_limit = "99999999999999999999999999999999999999999999999999999"]
//#![recursion_limit = "2056"]

macro_rules! explode {
    (recurse; $head:tt $($tail:tt)*) => { explode!(recurse; $($tail)*); };
    (recurse;) => {};
    
    (0; $($tt:tt)*) => { explode!(1; $($tt)* $($tt)* $($tt)*); };
    (1; $($tt:tt)*) => { explode!(2; $($tt)* $($tt)* $($tt)*); };
    (2; $($tt:tt)*) => { explode!(3; $($tt)* $($tt)* $($tt)*); };
    (3; $($tt:tt)*) => { explode!(recurse; $($tt)* $($tt)* $($tt)*); };
}

explode!(0; a b c);

This is because we don't check for integer overflow errors when parsing limits:

fn update_limit(krate: &ast::Crate, limit: &Once<usize>, name: Symbol, default: usize) {
for attr in &krate.attrs {
if !attr.check_name(name) {
continue;
}
if let Some(s) = attr.value_str() {
if let Some(n) = s.as_str().parse().ok() {
limit.set(n);
return;
}
}
}
limit.set(default);
}

There's a few options here. My preference would be to silently default to usize::MAX when overflow occurs.

This issue has been assigned to @fisherdarling via this comment.

@ecstatic-morse ecstatic-morse added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-attributes Area: Attributes (`#[…]`, `#![…]`) P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: Constant evaluation (MIR interpretation) and removed A-const-eval Area: Constant evaluation (MIR interpretation) labels Dec 12, 2019
@jonas-schievink jonas-schievink added the C-bug Category: This is a bug. label Dec 12, 2019
@fisherdarling
Copy link
Contributor

Hi! I'd like to try and handle this. If I understand this correctly, we should check if there is an ParseIntError, and if that error is an overflow. If it's an overflow, we default to usize::MAX?

@rustbot claim

@rustbot rustbot self-assigned this Dec 13, 2019
@estebank
Copy link
Contributor

@fisherdarling correct

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 17, 2020
…ow554

recursion_limit parsing handles overflows

This PR adds overflow handling to `#![recursion_limit]` attribute parsing. If parsing the given value results in an `IntErrorKind::Overflow`, then the recursion_limit is set to `usize::max_value()`.

closes rust-lang#67265
JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 18, 2020
…ow554

recursion_limit parsing handles overflows

This PR adds overflow handling to `#![recursion_limit]` attribute parsing. If parsing the given value results in an `IntErrorKind::Overflow`, then the recursion_limit is set to `usize::max_value()`.

closes rust-lang#67265
@bors bors closed this as completed in 6c4f859 Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants