-
Notifications
You must be signed in to change notification settings - Fork 76
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
derive: Protect against unbounded recursion when u.is_empty()
#109
Conversation
c8d4708
to
3e3940c
Compare
3e3940c
to
3d6b220
Compare
I'll publish a new release with this fix. |
Published 1.1.1 |
|
||
count.set(count.get() + 1); | ||
let result = { #expr }; | ||
count.set(count.get() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work as intended, i.e. is it ok to skip decreasing count if #expr
aborts through ?
?
Generated code for my struct, for consideration
fn arbitrary(u: &mut Unstructured) -> Result<Self> {
RECURSIVE_COUNT_Nah.with(|count| {
if count.get() > 0 && u.is_empty() {
return Err(arbitrary::Error::NotEnoughData);
}
count.set(count.get() + 1);
let result = {
Ok(
match (u64::from(<u32 as Arbitrary>::arbitrary(u)?) * 2u64) >> 32 {
0u64 => Nah::Foo(
Arbitrary::arbitrary(u)?,
Arbitrary::arbitrary(u)?,
Arbitrary::arbitrary(u)?,
),
1u64 => Nah::Bar(Arbitrary::arbitrary(u)?, Arbitrary::arbitrary(u)?),
_ => panic!("internal error: entered unreachable code"),
},
)
};
count.set(count.get() - 1);
result
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it further, wouldn't it be better to only modify count
when the unstructured is already empty?
- No need to access the thread local under normal conditions (performance micro-optimization, I know.)
- It's entirely possible that a recursive struct is benign and does finish generating on zeros only (after recursing a few times from the
Unstructured
input). But this currently aborts before trying that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to review a pull request if you want to make one!
Fixes #107