api.Memory.Size() overflows when the memory has the maximum 65536 pages

[EclesioMeloJunior]

Describe the bug
Basically, when calling memory.Grow function passing a delta that grows the memory to the MemoryLimitPages (65536) returns successfully but when we call memory.Size() the number of pages mismatches the expected.

I think that occurs because uint32 cannot fit the new buffer len (since the current page is 65536 and the number of bytes per page is 65536 and the multiplication of these values overflows uint32)

To Reproduce
I've wrote a test that reproduces the error in this fork ChainSafe#4

Expected behavior
Since the limit is MemoryLimitPages we should be able get the correct page size when growing up to that size

Screenshots
N/A

Environment (please complete the relevant information):

• Go version: 1.21.3
• wazero Version: 6a7e474
• Host architecture: arm64
• Runtime mode:

Additional context
Add any other context about the problem here.

[mathetake]

Thank you @EclesioMeloJunior for reporting this, and I confirmed that this is actually a bug in our API. As you described, when the memory has the maximum of 65536 pages, the buffer length becomes 2^32, which is larger than the maximum of uint32 which is the return type of api.Memory Size.

We could do either

1. Changing the return type of api.Memory Size to uint64
   • This is somewhat breaking change, though the impact should be really minimum because this is just a integer type conversion.
2. Adds another for SizeSafe() uint64 or whatever it is while putting deprecation comments on the api.Memory Size.
   • This won't be a breaking change

thoughts? @evacchi @ncruces @achille-roussel

[EclesioMeloJunior]

@mathetake thanks for the response, I have a question regards the SizeSafe, what it will return in a case where the buffer size overflows uint32 limits? Also, I would like to add that wasmtime memory size function returns a u64

I would like to add a third option:

• what about checking in the Grow method for this limit? I mean, instead of allowing the buffer size to reach 2^32 (which overflows uint32 only by 1) check and limit the buffer size/cap to be (2^32) - 1 then it will bound to uint32 limits (and the Size will not have problems to calc/return the correct size). I would assume that the problem with this approach is that the latest page will lose 1 byte (65535 instead of 65536), right?

[mathetake]

what about checking in the Grow method for this limit? I mean, instead of allowing the buffer size to reach 2^32 (which overflows uint32 only by 1) check and limit the buffer size/cap to be (2^32) - 1 then it will bound to uint32 limits (and the Size will not have problems to calc/return the correct size). I would assume that the problem with this approach is that the latest page will lose 1 byte (65535 instead of 65536), right?

no that's not our option as that means wazero is not compliant with the Wasm spec.

[ncruces]

Since memory size can't be (2^32) - 1, I'd return that and document the corner case?
I'd maybe call the other method Size64, in preparation for 64 bit memories?

[EclesioMeloJunior]

@mathetake @ncruces thanks for the clarifications, I took the freedom to create a draft PR that introduces the new method #1856

[mathetake]

So we are looking to release wazero 2.0 next month, and I think we can make the breaking change to this rather than having another confusing api like Size64.

Tomorrow I will raise a PR to do so, which supersedes #1856. Thank you for your help anyway @EclesioMeloJunior

[mathetake]

reopening this as we've decided to not fix this in 2.0, which is explained in detail in #2106

[ncruces]

Maybe we could document the issue, and add a method Pages() that allows you to figure out the difference between 0 and 65536 pages.

[ncruces]

I think we should:

1. document the issue here.
2. note that you can obtain the number of pages of the memory by calling Grow(0).
3. possibly add the already implemented PageSize() method to the interface.

Then we can close this? I mean, given the above, I guess the biggest deficiency is not the Size() method, rather that it's not possible for Read() to ever give you the full 4GB buffer.

[mathetake]

regardless of having new API or not, defer to @evacchi for any decision from now!