rust-fuzz / arbitrary

Generating structured data from arbitrary, unstructured input.
https://docs.rs/arbitrary/
Apache License 2.0
730 stars 75 forks source link

Failure to build 1.4.1 on existing projects that were using 1.3.2 #208

Open leighmcculloch opened 2 weeks ago

leighmcculloch commented 2 weeks ago

Similar to #203, it seems like the 1.4.1 releases are still making assumptions about items in the current space that weren't prior, which is a breaking change, and fails to build on projects who were dependent on 1.3.2.

Cargo.toml:

[package]
name = "pkg"
edition = "2021"

[dependencies]
soroban-sdk = { version = "=21.7.6" }

[dev-dependencies]
soroban-sdk = { version = "=21.7.6", features = ["testutils"] }

src/lib.rs:

#![no_std]

use soroban_sdk::contracttype;

#[contracttype]
#[derive(Clone, Debug, PartialEq)]
pub struct Struct {
    pub index: u64,
}

Run cargo test with 1.4.1:

$ cargo update artbirary --precise 1.4.1
$ cargo update derive_artbirary --precise 1.4.1
$ cargo test

Included in the output are the following build errors:

error[E0433]: failed to resolve: could not find `std` in the list of imported crates
 --> src/lib.rs:5:1
  |
5 | #[contracttype]
  | ^^^^^^^^^^^^^^^ could not find `std` in the list of imported crates
  |
  = note: this error originates in the derive macro `soroban_sdk::testutils::arbitrary::arbitrary::Arbitrary` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0425]: cannot find value `RECURSIVE_COUNT_ArbitraryStruct` in this scope
 --> src/lib.rs:5:1
  |
5 | #[contracttype]
  | ^^^^^^^^^^^^^^^ not found in this scope
  |
  = note: this error originates in the derive macro `soroban_sdk::testutils::arbitrary::arbitrary::Arbitrary` (in Nightly builds, run with -Z macro-backtrace for more info)

Re-run cargo test with 1.3.2 and it compiles:

$ cargo update artbirary --precise 1.3.2
$ cargo update derive_artbirary --precise 1.3.2
$ cargo test
fitzgen commented 2 weeks ago

Can you provide a standalone and minimal test case that reproduces the issue? That would be helpful in diagnosing and fixing the bug.

Manishearth commented 2 weeks ago

I suspect it's the use of std::thread_local in the recursion counter.

I think folks should just #[cfg_attr(feature = "std", derive(Arbitrary))] or something (or #[cfg(feature = "arbitrary)] extern crate std;). I don't think this crate ever guaranteed that the derive works in no_std. We potentially could try to but I don't see a clean way of doing so without losing the recursion guard.

Manishearth commented 2 weeks ago

Recursion guard was added in https://github.com/rust-fuzz/arbitrary/pull/109

leighmcculloch commented 1 week ago

Up until the 1.4 releases we've been able to support arbitrary in no_std crates by extern importing std into the space like so:

soroban-sdk-macros/src/arbitrary.rs:

        const _: () = {
            // derive(Arbitrary) expects these two to be in scope
            use #path::testutils::arbitrary::std;
            use #path::testutils::arbitrary::arbitrary;

            #[derive(#path::testutils::arbitrary::arbitrary::Arbitrary)]
            #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
            #vis #arbitrary_type_decl

            impl #path::testutils::arbitrary::SorobanArbitrary for #ident {
                type Prototype = #arbitrary_type_ident;
            }

            impl #path::TryFromVal<#path::Env, #arbitrary_type_ident> for #ident {
                type Error = #path::ConversionError;
                fn try_from_val(env: &#path::Env, v: &#arbitrary_type_ident) -> std::result::Result<Self, Self::Error> {
                    Ok(#arbitrary_ctor)
                }
            }
        };

Ref: https://github.com/stellar/rs-soroban-sdk/blob/0c65e9c9866905808a18ff97ec54be08da986bc3/soroban-sdk-macros/src/arbitrary.rs#L320-L339

soroban-sdk/src/testutils/arbitrary.rs:

// Used by `contracttype`
#[doc(hidden)]
pub use std;

Ref: https://github.com/stellar/rs-soroban-sdk/blob/0c65e9c9866905808a18ff97ec54be08da986bc3/soroban-sdk/src/testutils/arbitrary.rs#L177-L179

It seems something has changed where I guess the arbitrary derive is now creating a new scope, that isn't seeing the std we are including into the derive scope.

I think this basically breaks arbitrary for any no_std crate, where it worked before as long as the crate imported std into the scope being derived into.