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

Improve derived recursion guard #111

Merged
merged 2 commits into from
Jun 23, 2022
Merged

Improve derived recursion guard #111

merged 2 commits into from
Jun 23, 2022

Conversation

jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Jun 17, 2022

As per your invitation.

Turns

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
    })
}

into

fn arbitrary(u: &mut Unstructured<'arbitrary>) -> Result<Self> {                                                                                                          
    let guard_against_recursion = u.is_empty();                                                                      
    if guard_against_recursion {                                                                                                   
        RECURSIVE_COUNT_Nah.with(|count| {                                                                                                      
            if count.get() > 0 {                                                                                                                               
                return Err(arbitrary::Error::NotEnoughData);       
            }                                                                                                                 
            count.set(count.get() + 1);                                                                                                      
            Ok(())                                                                                                                                          
        })?;                                                                                                                                                               
    }                                                                                                                      
    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)?),                                                  
                _ => ::core::panicking::panic("internal error: entered unreachable code"),                                                          
            },                                                                                                                                                     
        )                                                                                                          
    })();                                                                                                                         
    if guard_against_recursion {                                                                                                                 
        RECURSIVE_COUNT_Nah.with(|count| {                                                                                                                    
            count.set(count.get() - 1);        
        });                                                                                                                  
    }                                                                                                                                       
    result                                                                                                                                                 
}     

I've also wrapped the thread local and impl in a const _: () = {…}; to avoid name clashes. I have no idea whether that's the best way of doing it, but it's what serde does.

@fitzgen
Copy link
Member

fitzgen commented Jun 21, 2022

I've also wrapped the thread local and impl in a const _: () = {…}; to avoid name clashes.

What name clashes are possible, given that we add the type name to the recursion guard's name?

})?;
}

let result = (|| { #expr })();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the immediately-invoked closure here to allow passing things like return 42 as expr?

Copy link
Contributor Author

@jcaesar jcaesar Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda. It is to stop ? in #expr from aborting the outer function before the recursion counter has been decreased again. Ugly hack in the absence of try blocks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks, makes sense.

@jcaesar
Copy link
Contributor Author

jcaesar commented Jun 21, 2022

What name clashes are possible, given that we add the type name to the recursion guard's name?

#[derive(Arbitrary)]
struct Foo;

struct RECURSIVE_COUNT_Foo: i32 = 42;

Sure, unlikely that anyone runs into this on accident, but since it's easy to prevent…

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fitzgen fitzgen merged commit 54c7b11 into rust-fuzz:master Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants