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

[x/programs] Improve overflow checks #600

Merged
merged 15 commits into from
Nov 17, 2023
Merged

[x/programs] Improve overflow checks #600

merged 15 commits into from
Nov 17, 2023

Conversation

samliok
Copy link
Contributor

@samliok samliok commented Nov 2, 2023

This PR improves overflow handling and generally improves developer UX by allowing not restricting Call params sent to the Program to be uint64.

x/programs/runtime/runtime.go Outdated Show resolved Hide resolved
@samliok
Copy link
Contributor Author

samliok commented Nov 2, 2023

I think we should refactor how we handle uint64 & int64. I updated the PR to reduce our risk of overflow and allowing greater functionality with signed integers.

In the codebase we were restricting call params to our wasm programs to only uint64 types. This approach doesn't make sense because

  • wasm does not have unsigned integer types and will just convert our params into either i32 or i64
  • this artificially limits us from passing negative values into wasm.
  • potential unchecked overflow errors as max uint64 > max int64

Because of WASM's nature we should default to using int64.

@samliok
Copy link
Contributor Author

samliok commented Nov 2, 2023

In addition to #429 we need to make sure our wrapper u64 does not exceed MaxInt64

@samliok samliok changed the title [x/programs] Allow #[public] functions to return nothing [x/programs] Use int64 in host & allow #[public] functions to return nothing Nov 3, 2023
@samliok samliok changed the title [x/programs] Use int64 in host & allow #[public] functions to return nothing [x/programs] Use int64 in host Nov 3, 2023
x/programs/runtime/dependencies.go Show resolved Hide resolved
x/programs/runtime/memory.go Show resolved Hide resolved
x/programs/examples/util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

This makes sense one last nit 🙏

  • ensure safe conversion from int64 to int32

Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

very close!

x/programs/runtime/memory.go Outdated Show resolved Hide resolved
x/programs/runtime/consts.go Outdated Show resolved Hide resolved
x/programs/runtime/runtime.go Outdated Show resolved Hide resolved
x/programs/runtime/dependencies.go Outdated Show resolved Hide resolved
x/programs/examples/util.go Outdated Show resolved Hide resolved
@samliok
Copy link
Contributor Author

samliok commented Nov 10, 2023

@hexfusion should I also check the return value for overflow in this function?

pub extern "C" fn alloc(len: usize) -> *mut u8 {

@hexfusion
Copy link
Contributor

hexfusion commented Nov 10, 2023

@hexfusion should I also check the return value for overflow in this function?

pub extern "C" fn alloc(len: usize) -> *mut u8 {

Well we should ensure that all the conversions are safe for Alloc. I think we still need to check uint64 -> int32 here in golang? Not sure how practical that overflow would be but doesn't hurt.

func (m *memory) Alloc(length uint64) (uint64, error) {
fn, err := m.client.ExportedFunction(AllocFnName)
if err != nil {
return 0, err
}
result, err := fn.Call(m.client.Store(), int32(length))

the export to alloc takes an i32 as param and usize can't be negative so I am not sure if something strange could happen there I suppose you could check against i32::MAX but it might not be nessisary.

// ensure memory pointer is fits in an i64
// to avoid potential issues when passing
// across wasm boundary
assert!(i64::try_from(ptr as u64).is_ok());
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@hexfusion hexfusion changed the title [x/programs] Use int64 in host [x/programs] Improve overflow checks Nov 17, 2023
@hexfusion hexfusion merged commit 283846b into main Nov 17, 2023
22 checks passed
@hexfusion hexfusion deleted the accept_nil branch November 17, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasm runtime Wasm runtime
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants