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

A test that triggers null pointer exception on 3rd level of external query #212

Closed
wants to merge 2 commits into from
Closed

A test that triggers null pointer exception on 3rd level of external query #212

wants to merge 2 commits into from

Conversation

assafmo
Copy link
Contributor

@assafmo assafmo commented Jul 21, 2020

Also need to add a print in this line: https://github.com/CosmWasm/go-cosmwasm/blob/master/api/callbacks.go#L415

fmt.Printf("\n\n\ncQueryExternal: gasAfter: %d gasBefore: %d querier: %v\n\n\n", gasAfter,gasBefore, querier)

And then you'd see that gasAfter == gasBefore and that in the 3rd level of query that the vTable at the end of querier is {<nil> <nil> <nil> <nil>}, which leads to null pointer exception later on in keeper.go/QuerySmart.

@assafmo assafmo requested a review from ethanfrey as a code owner July 21, 2020 16:02
@assafmo assafmo changed the title create the test A test that triggers null pointer exception on 3rd level of external query Jul 21, 2020
@assafmo
Copy link
Contributor Author

assafmo commented Jul 22, 2020

Fixed by #213

@assafmo assafmo closed this Jul 22, 2020
@ethanfrey
Copy link
Member

I will review this. I think it would be helpful to add this (or a modified version) to wasmd to ensure we have no regressions. As the fix had no test with it.

Essentially don't delete the branch, I will bring it into master with possibly a different contract

@assafmo
Copy link
Contributor Author

assafmo commented Jul 22, 2020

Okay, for sure.
Just note that the original test was intended to test that the SGX OOM killer handles infinite external query loops gracefully, so you might want to create a different test that e.g. passes a number+1 to the next query and when it reaches 10 or 20 just return, and make sure that the first caller receives 10 or 20.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub enum QueryMsg {
    SendExternalQueryInfiniteLoop { to: HumanAddr, counter: u64 },
}

pub fn query<S: Storage, A: Api, Q: Querier>(deps: &Extern<S, A, Q>, msg: QueryMsg) -> QueryResult {
    match msg {
        QueryMsg::SendExternalQueryInfiniteLoop { to, counter } => {
            send_external_query_infinite_loop(deps, to, counter)
        }
    }
}

fn send_external_query_infinite_loop<S: Storage, A: Api, Q: Querier>(
    deps: &Extern<S, A, Q>,
    contract_addr: HumanAddr,
    counter: u64,
) -> QueryResult {
    if counter >= 10 {
        return Ok(Binary(counter.into()))
    }

    let answer = deps
        .querier
        .query::<u64>(&QueryRequest::Wasm(WasmQuery::Smart {
            contract_addr: contract_addr.clone(),
            msg: Binary(
                format!(
                    r#"{{"send_external_query_infinite_loop":{{"to":"{}","counter":{}}}}}"#,
                    contract_addr.clone().to_string(),
                    counter + 1,
                )
                .into(),
            ),
        }));

    match answer {
        Ok(counter) => Ok(Binary(counter.into())),
        Err(e) => Err(e),
    }
}

@assafmo
Copy link
Contributor Author

assafmo commented Jul 22, 2020

And in go code:

counter, err = keeper.QuerySmart(ctx, contractAddress, []byte(fmt.Sprintf(`{"send_external_query_infinite_loop":{"to":"%s","counter":1}}`, contractAddress.String())))
require.Equal(t, 10, counter)

(Probably need to convert counter from bytes to uint64)

@ethanfrey
Copy link
Member

Thanks, I will add such to the hackatom contract

1 similar comment
@ethanfrey
Copy link
Member

Thanks, I will add such to the hackatom contract

zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
Use new --keyring-backend flag.

See cosmos/cosmos-sdk#5355 for more information.
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.

2 participants