Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Sep 10, 2019

There is a problem with read_storage, if an offset passed that is out of bounds of the length of the value the implementation of an extrinsic will panic. This PR solves it.

There is another problem with the API itself: there is no way to indicate how many bytes were read. So even if we fix the panic, the API wouldn't be universally usable since the API cannot indicate how many bytes it wrote to the buffer.

The API is not used within Substrate, except for implementing exists (e.g. this).

@pepyakin pepyakin marked this pull request as ready for review September 11, 2019 12:36
@pepyakin pepyakin requested a review from bkchr September 11, 2019 12:36
@pepyakin pepyakin added the A0-please_review Pull request needs code review. label Sep 11, 2019
@gavofyork
Copy link
Member

gavofyork commented Sep 11, 2019

alternative here: #3597 which i prefer since it just forces a cap on the value rather than introducing a full conditional.

the API cannot indicate how many bytes it wrote to the buffer

let offset = /* whatever offset into the value we want to read from */;
let mut out = [0u8; /* whatever the buffer size is */]; 
if let Some(value_len) = read_storage(key, &mut out, offset) {
    // offset will be truncated to the value's size.
    let offset = offset.min(value_len);
    // number of bytes written is the size of the value minus any offset into it that we start at
    // and it will never be more than the size of the output buffer.
    let bytes_written = out.len().min(value_len - offset);
}

@pepyakin
Copy link
Contributor Author

Ah, but in this case we don't return the original value's length here which panics with the following error

  left: `Some(0)`,
 right: `Some(4)`', /builds/parity/substrate/core/test-runtime/src/lib.rs:919:2

@gavofyork
Copy link
Member

should do the trick.

@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Sep 11, 2019
@gavofyork gavofyork merged commit 15a4c40 into master Sep 11, 2019
@gavofyork gavofyork deleted the ser-fix-offset branch September 11, 2019 17:37
andresilva pushed a commit that referenced this pull request Sep 17, 2019
* Add a failing test case

* Actual fix

* read_child_storage, fix wasm side

* Bump the impl version.

* Alternative (#3597)

* Update with_std.rs

* Update with_std.rs

* Update wasm_executor.rs

* Update wasm_executor.rs

* Update with_std.rs

* Update wasm_executor.rs
Demi-Marie pushed a commit to Demi-Marie/substrate that referenced this pull request Sep 17, 2019
* Add a failing test case

* Actual fix

* read_child_storage, fix wasm side

* Bump the impl version.

* Alternative (paritytech#3597)

* Update with_std.rs

* Update with_std.rs

* Update wasm_executor.rs

* Update wasm_executor.rs

* Update with_std.rs

* Update wasm_executor.rs
en pushed a commit to en/substrate that referenced this pull request Sep 24, 2019
* Add a failing test case

* Actual fix

* read_child_storage, fix wasm side

* Bump the impl version.

* Alternative (paritytech#3597)

* Update with_std.rs

* Update with_std.rs

* Update wasm_executor.rs

* Update wasm_executor.rs

* Update with_std.rs

* Update wasm_executor.rs
@bkchr bkchr mentioned this pull request Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants