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

Stack overflow instantiating deeply nested ScVecs, etc. #861

Closed
brson opened this issue Jun 16, 2023 · 2 comments · Fixed by #904
Closed

Stack overflow instantiating deeply nested ScVecs, etc. #861

brson opened this issue Jun 16, 2023 · 2 comments · Fixed by #904
Assignees
Labels
bug Something isn't working Security Security fixes or features

Comments

@brson
Copy link
Contributor

brson commented Jun 16, 2023

Instantiating deeply-nested ScVecs into the host environment causes stack overflow. Presumably also with maps and UDTs. It is unclear to me whether this is something that a user can trigger by e.g. submitting a specially-crafted transaction.

I am interested in opinions on whether this is something exposed to user input, how to test whether it is exposed to user input, and whether it should be fixed.

What version are you using?

e6e02dc

What did you do?

Run this test case

#[test]
fn deep_vec() -> Result<(), HostError> {
    let host = Host::default();

    let mut v = ScVec::default();
    for _ in 0..1000 {
        let vv = ScVec::try_from(vec![ScVal::from(v)]).unwrap();
        v = vv;
    }

    // stack overflow here
    let _ = host.to_host_val(&ScVal::from(v));

    Ok(())
}

What did you expect to see?

An error.

What did you see instead?

Stack overflow.

@brson brson added the bug Something isn't working label Jun 16, 2023
@brson
Copy link
Contributor Author

brson commented Jun 16, 2023

I think conversion in the opposite direction, from_host_val can also stack overflow.

A guest can trigger these with the put_contract_data and get_contract_data syscalls. Doing so requires a large budget, but someone clever might be able to do it in increments.

Here's a guest-side contract that stack overflows the host with budget turned off:

#[contractimpl]
impl FuzzContract {
    pub fn run(env: Env) {
        use soroban_sdk::{Vec, Symbol};

        let mut v: Vec<RawVal> = Vec::new(&env);
        for _ in 0..10000 {
            let mut vv: Vec<RawVal> = Vec::new(&env);
            vv.push_back(v.into());
            v = vv;
        }

        let key = Symbol::short("key");
        env.storage().set(&key, &v);
        env.storage().get::<_, Vec<RawVal>>(&key);
    }
}

This overflows during the storage().set call, but if that succeeded it would probably also overflow in storage().get

@brson
Copy link
Contributor Author

brson commented Jun 26, 2023

I think it's probably possible to stack-overflow the XDR deserializer as well. This looks unboundedly recursive:

impl<T: ReadXdr, const MAX: u32> ReadXdr for VecM<T, MAX> {
    #[cfg(feature = "std")]
    fn read_xdr(r: &mut impl Read) -> Result<Self> {
        let len = u32::read_xdr(r)?;
        if len > MAX {
            return Err(Error::LengthExceedsMax);
        }

        let mut vec = Vec::with_capacity(len as usize);
        for _ in 0..len {
            let t = T::read_xdr(r)?;
            vec.push(t);
        }

        Ok(VecM(vec))
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Security Security fixes or features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants