Keep track of pointers and their length with a custom store #857
Conversation
iFrostizz
requested review from
samliok,
dboehm-avalabs and
richardpringle
as code owners
| let val = into_bytes(ptr as i64).unwrap(); | ||
| assert_ne!(val, vec); | ||
| assert!(GLOBAL_STORE.with_borrow(|s| s.get(&(ptr as *const u8)).is_none())); | ||
| } |
There was a problem hiding this comment.
🤔 this isn't really testing anything that we're doing. In other words, I'm not sure that there's any code outside of this test that you could change to only make this test fail.
There was a problem hiding this comment.
I figured that this test was probably much more straightforward than writing a broken allocator to do that but that wouldn't be that useful anyway, so I will just delete this test
| fn big_allocation_fails() { | ||
| // see https://doc.rust-lang.org/1.77.2/std/alloc/struct.Layout.html#method.array | ||
| alloc(isize::MAX as usize + 1); | ||
| } |
There was a problem hiding this comment.
You're testing over the boundary here, what about on the boundary? (This test is good, I think you need another one).
There was a problem hiding this comment.
What happens when you allocate more than half the space twice? Should probably test this as well.
There was a problem hiding this comment.
Definitely a good idea! Adding this one
There was a problem hiding this comment.
Same issue, the allocation does fail but still abort. It's really great that you found that the null pointer is returned when the allocation failed though !
There was a problem hiding this comment.
🤔 Maybe we save the tests that abort for an integration test.
|
There is an irrefutable pattern here hypersdk/x/programs/examples/imports/pstate/pstate.go Lines 159 to 164 in 897be9b Please thank golsp ! 
|
|
|
| fn into_bytes(ptr: HostPtr) -> Option<Vec<u8>> { | ||
| GLOBAL_STORE | ||
| .with_borrow_mut(|s| s.remove(&(ptr as *const u8))) | ||
| .map(|len| unsafe { std::vec::Vec::from_raw_parts(ptr as *mut u8, len, len) }) |
There was a problem hiding this comment.
In your next PR, can you also make sure to leave a // Safety: comment on all the unsafe blocks?
Closes #846, closes #845
Changelog
HostPtrtypeSmartPtron the Go sidememorymodule