Linter PR 6: eliminate usage of unsafe

[faddat]

This pull request makes the use of unsafe code much less common in wasmvm, and makes a number of other tightenings and enhancements.

Chat log from chat with Skip/ICL

I've made a memory safe unmanaged vector
3:57
I know lots of people said it is impossible, but well, that's done and that would get us huge breathing room for new implementations of all kinds of things
3:58
like IBCPacket recieve
3:58
I just fixed the unmanaged vector issues there.
3:58
it now uses SafeUnmanagedVerctor
3:59
this is like, unambiguously good
4:00
Key safety features of SafeUnmanagedVector:
It tracks whether the vector has been consumed with a consumed boolean flag
The consume() method checks this flag and returns an error if the vector has already been consumed
It provides safer methods like is_consumed() to check the consumption state
It has helper methods to safely create, consume, and destroy vectors
The FFI interface provides functions like:
new_safe_unmanaged_vector: Creates a SafeUnmanagedVector that tracks consumption
destroy_safe_unmanaged_vector: Safely destroys a SafeUnmanagedVector, preventing double-free
safe_unmanaged_vector_is_none: Checks if a vector is None without consuming it
safe_unmanaged_vector_length: Gets vector length safely
safe_unmanaged_vector_to_bytes: Copies content to a Go byte slice
This implementation provides a much safer alternative to the original UnmanagedVector by preventing common memory safety issues like double-free that can occur when working with FFI between Rust and Go.


Jacob Gadikian
  4:07 PM
Updated all IBC-related functions to use CopyAndDestroyToSafeVector instead of copyAndDestroyUnmanagedVector:
IBCChannelOpen, IBCChannelConnect, IBCChannelClose
IBCPacketReceive, IBC2PacketReceive, IBCPacketAck, IBCPacketTimeout
IBCSourceCallback, IBCDestinationCallback
Updated error handling in StoreCode, StoreCodeUnchecked, and errorWithMessage to use the safer approach.
Updated analysis and metrics functions to use the safer approach:
receiveAnalysisReport - Now safely handles required_capabilities and entrypoints
receivePinnedMetrics - Now uses SafeUnmanagedVector for the metrics data
4:07
now these would really solve serious security issues.
4:07
These changes make the codebase more consistent in its approach to memory management. The safer patterns help prevent:
Memory leaks - By ensuring proper cleanup of allocated memory
Double-free issues - By tracking whether a vector has been consumed already
Use-after-free bugs - By preventing access to memory after it has been freed
4:08
all of that started just with linting rigorously


Jacob Gadikian
  4:17 PM
The bug has been fixed by replacing the direct json.Unmarshal() calls with DeserializeResponse() in the Instantiate, Execute, and Query methods.
The fix ensures that:
Gas costs are properly accounted for when deserializing response data
Gas is charged based on the size of the response
Gas reports are properly updated
This prevents the potential DoS vulnerability that could have been exploited by returning large responses without incurring the correct computational cost.
4:17
etc etc etc etc
4:18
In the UnmarshalJSON method for VoteMsg, a nil pointer dereference can occur if both "option" and "vote" fields are missing in the input JSON. When tmp.Option remains nil, attempting to assign *tmp.Option to Option will cause a panic. The method should add a check to handle cases where neither field is provided, either by returning an error or setting a default value.
4:19
I could be wrong but that could be a take-out-any-chain issue
4:20
The Close() method of memDBIterator introduces a potential resource leak by asynchronously draining the channel. By spawning a separate goroutine to drain the channel, Close() returns immediately before the underlying traversal goroutine completes, which may cause the database's read lock to remain held longer than expected. This can potentially lead to resource leaks or deadlocks if callers assume Close() synchronously releases all resources.
...etc etc etc
4:21
Ricardo will be reaching out to figure out billing or something.  This needs to happen -- not just getting paid, but these improvements.  Wasmvm is just much too uncool right now, but it is savable
4:21
and then we can rewrite it
4:22
I just fixed all 3 of those
4:23
this is my attempting to show good faith here - I just want to get this stuff fixed so we can go on and moon without explosions theres been too many

[chipshort]

I will not review a 7000+ additions PR that's a total mess of all kinds of changes. I simply do not have time for that. Please keep your PRs separate unless it's absolutely necessary for them to depend on each other.
Looking at some of the functions you wrote here, I am not convinced this is as big of an improvement as you make it out to be, so I will not review further until there is a minimal PR with only the necessary changes for your SafeUnmanagedVector idea.

[chipshort]

This is complete nonsense and doesn't make this safer at all. You start out by creating a new SafeUnmanagedVector with consumed set to false, so your whole codepath just boils down to calling safe_vector.inner.consume() anyways, just with a lot of unnecessary fluff around it.

[faddat]

this is what I needed!

thank you!

[chipshort]

None of the uses of CopyAndDestroyToSafeVector make sense here. You always just call ToBytesAndDestroy immediately anyways. In that case, you might as well just use the existing copyAndDestroyUnmanagedVector directly, which returns perfectly safe []byte to you.
Using CopyAndDestroyToSafeVector gives no additional safety at all. Nothing stops you from calling it twice and therefore double-freeing.

[faddat]

I was worried about this.

[faddat]

hey @pinosu -- to be clear, this is exactly the kind of guidance I needed!

I will look into the rest, no worries, but I also want to provide you with some guidance -- the critical-path PR, that I must ask that you review is #589 , and reviewing that will greatly reduce the scope of others.

But in the interests of getting things done, and because I know the dangers of giant crazy PRs I'm going to break that down into other PRs, which work exactly linter per linter. But pelase, treat 589 as the critical one because we need to ensure that we don't do stupid things, that's really all the linter does.

I found a lot of them but that's no problem everyone does stupid things from time to time just please don't take it as an affront because it isn't meant that way.

[faddat]

the worst part is that on second thought you really may want to review this PR.

Tangled up in my linting I found a lot of stuff that's definitely a bug.

[faddat]

Sorry, input-validation 2

PLEASE READ IT IT CONTAINS THINGS THAT ARE BAD THANK YOU LAST FEW COMMITS