runtime: Add null pointer exception for read #2780
Conversation
evaporei
force-pushed
the
otavio/add-null-ptr-message
branch
from
5c566ef
to
911dcc3
Compare
evaporei
force-pushed
the
otavio/add-null-ptr-message
branch
from
911dcc3
to
dea9f03
Compare
| "Subtract overflow on pointer: {}", // Usually when pointer is zero because of null in AssemblyScript | ||
| self.0 | ||
| )) | ||
| DeterministicHostError(anyhow::anyhow!("Subtract overflow on pointer: {}", self.0)) |
There was a problem hiding this comment.
Could we say something more helpful here (like what that means in terms of pointers)? Is this always an internal error? And it seems that size_of_rt_size could be const.
There was a problem hiding this comment.
You're correct, the size of rt_size can be changed to a const, I'll change that.
This Subtract overflow shouldn't happen, it's related to the way every type is layed out in memory.
All class instances in AssemblyScript have a header of 20 bytes, of which the last 4 hold the size of the following data.
AS header memory layout docs: https://www.assemblyscript.org/memory.html#common-header-layout.
Our AscPtr structure holds a number which is the address in WebAssembly memory of where that class instance starts their data (right after the header).
So this part of the code tries to do a pointer arithmetic to get the size of the structure, so it can read it fully in a correct fashion.
Maybe what I can write instead of this message is that it failed to find the size of the following structure.
The part about being an internal/external error, in this case it actually can appear in the explorer logs, like Adam found them. It will either happen when we messed up (me in this case, cause I did this code haha) the AS memory layout, or AS is writing data to memory that doesn't conform to the especification.
There was a problem hiding this comment.
I added the constant but I'll let these (there are more) "Subtract overflow" to fix later. I want to discuss more with @leoyvens on this. Thanks for your input!
graph/src/runtime/asc_ptr.rs
Outdated
| /// this function does this error handling. | ||
| /// | ||
| /// Summary: ALWAYS call this before reading an AscPtr. | ||
| pub fn assert_is_not_null(&self) -> Result<(), DeterministicHostError> { |
There was a problem hiding this comment.
Minor, but it would be better to call this check_is_not_null, when I see assert I expect that the code could panic.
There was a problem hiding this comment.
Makes sense! I'll change this rn :)
evaporei
force-pushed
the
otavio/add-null-ptr-message
branch
from
dea9f03
to
821e337
Compare
evaporei
force-pushed
the
otavio/add-null-ptr-message
branch
from
a7bec0e
to
d7a17e7
Compare
This can be reviewed commit by commit 🙂
Before this PR users would get the
Subtract overflowerror in apiVersion0.0.5, by using operator overloading from types likeBigInt:This PR aims at making the error message more clear. Also there are tests for that case as well, the AS code for it should be self explanatory.
Also, this message will be avoided for operator overloading in
graph-tsjust like theSafeBigIntdid on the tests. PR for this: graphprotocol/graph-ts#204