Heap data corruption in scripts

[segfault-magnet]

While working on 1046 @hal3e and I noticed that vectors allocated and returned from a contract call have their data located below the current value of hp.

The heap is expanded during the contract call but the expansion is reverted after the call is finished.

This hp revert leaves the returned vector data in unallocated memory. If you allocate enough bytes and write to them you will overwrite the returned vector's data.

A minimal example can be seen here: https://github.com/segfault-magnet/sway_heap_issue, running doit.sh will spin a node, deploy the contract and replace the CONTRACT_ID placeholder with the actual contract id and proceed to run the script.

Tested on:

❯ forc --version
forc 0.42.1

~
❯ fuel-core --version
fuel-core 0.18.3

[Dentosal]

The specification says that $hp would be restored to it's value before CALL when returning with RET or RETD.

Then pop the call frame and restoring registers except $ggas, $cgas, $ret, and $retl.

The VM follows the spec here.

We could change the spec and not restore $hp on return. However, not reclaiming the heap space of the called context means that any callee using the heap would inflate the heap size forever, since it couldn't be reclaimed anymore.

Instead maybe the compiler could handle the memory being unwritable and also avoid allocating over it with ALOC, potentially copying it to heap preserving it using ALOC. This is complicated, however, and there might be better solutions to this. One way could be just adding DALO (dealloc instruction) so that the heap space is not lost so easily.

[vaivaswatha]

Instead maybe the compiler could handle the memory being unwritable and also avoid allocating over it with ALOC, potentially copying it to heap preserving it using ALOC. This is complicated, however, and there might be better solutions to this. One way could be just adding DALO (dealloc instruction) so that the heap space is not lost so easily.

This gets needlessly complex if we have multiple objects on the heap, and with some of them pointing internally to the others. I vote against this. The whole point of a heap is for it to be independent of the stack.

A dealloc instruction may help, but without an actual runtime to manage and reallocate deallocated memory it isn't that useful. This is equal to having a malloc within the VM (runtime). And the result of alloc should be an actual free memory address and not just a movement of $hp.

[Dentosal]

Agreed. The only way forwards without a big redesign is not restoring $hp on return.

Spec PR: FuelLabs/fuel-specs#506

[Dentosal]

Fixed in FuelLabs/fuel-vm#525. Should we keep this open while waiting for the fix to propagate through fuel-vm and fuel-core releases?

[vaivaswatha]

Fixed in FuelLabs/fuel-vm#525. Should we keep this open while waiting for the fix to propagate through fuel-vm and fuel-core releases?

Yes, let it be open until confirmation.

[vaivaswatha]

Assigning back to @segfault-magnet to verify and close.