Proposal Details
When using unsafe code to get the last bit of performance, in some cases we have an unsafe.Pointer to a slice that we use without checking bounds. We would like to check bounds in special ("invariant") builds, but in many code paths the bounds aren't available. It would be very useful if there was an 'unsafe.ObjectBounds(p unsafe.Pointer) (base unsafe.Pointer, length uintptr, ok bool)` that would return information about the heap object which overlaps the pointer.
GPT has a good starting point for the change, which seems straightforward: https://chatgpt.com/share/6875e6d7-91cc-8010-b261-a798a0337c5f
Comment From: seankhliao
I don't think this is a style of code that Go aims for.
Comment From: Jorropo
For no particular reason, because it's the third time I see this being done here.
Never link an LLM chat log that is multiple pages long and except anyone to understand what you mean, you must extract the 2 relevant lines of code and post the API in your issue.
Comment From: RaduBerinde
I did extract the relevant part, all the information is in the issue. The link contains details of how the implementation might look, for someone who would be implementing this.
Comment From: RaduBerinde
I don't think this is a style of code that Go aims for.
I'd argue this is true of unsafe code in general.. But within the unsafe package, this doesn't seem unreasonable.
Is there any other variation that would be more palatable? Checking that an unsafe.Pointer is valid? Checking that two unsafe.Pointers overlap the same underlying object?
Comment From: Jorropo
I did extract the relevant part, all the information is in the issue. The link contains details of how the implementation might look, for someone who would be implementing this.
This proposal is about adding a new function yet you do not provide it's signature or documentation which are extremely important.
Comment From: RaduBerinde
I did extract the relevant part, all the information is in the issue. The link contains details of how the implementation might look, for someone who would be implementing this.
This proposal is about adding a new function yet you do not provide it's signature or documentation which are extremely important.
I did provide its signature, and IMO it should be clear from the signature and the rest of the description what the function would do.
Comment From: Jorropo
I did provide its signature
~~There is no codeblock nor signature in this issue.~~
mb there is the signature but not the documentation, the best way is to make a codeblock
Comment From: seankhliao
I don't think any variant would be acceptable. unsafe enables you to bypass safeguards when you already have prior information from some other source that can prove the operation is safe.
what you've asked for is introspection of state because you don't know if you've written safe code.
Comment From: jbowens
I don't think any variant would be acceptable. unsafe enables you to bypass safeguards when you already have prior information from some other source that can prove the operation is safe.
what you've asked for is introspection of state because you don't know if you've written safe code.
Would a variant that throws an unrecoverable runtime panic if two pointers are not part of the same object truly be unpalatable? It could only be used to improve the safety of code.
The goal here is to improve safety by translating statically-known invariants into runtime verification of the invariants. I don't think runtime verification of static invariants is against the spirit of the unsafe package.
Comment From: RaduBerinde
Another idea: a new variant of unsafe.Add():
// CheckedAdd is like Add but panics if ptr does not correspond to
// a Go object on the heap or if the resulting pointer does not
// correspond to the same object.
func CheckedAdd(ptr unsafe.Pointer, len uintptr) unsafe.Pointer
Comment From: RaduBerinde
what you've asked for is introspection of state because you don't know if you've written safe code.
Correct - in almost all cases, anyone who writes any non-trivial piece of code does not know if they wrote correct code.
Comment From: thepudds
Another idea: a new variant of unsafe.Add():
// CheckedAdd is like Add but panics if ptr does not correspond to // a Go object on the heap or if the resulting pointer does not // correspond to the same object. func CheckedAdd(ptr unsafe.Pointer, len uintptr) unsafe.Pointer
Hi @RaduBerinde, FWIW, using -gcflags=all=-d=checkptr does already cause the runtime to dynamically check that unsafe pointer arithmetic on a heap object ends up resulting in a pointer that is in bounds for that starting object (or at least one of the unsafe.Pointer operands, if there are multiple).
See for example https://go.dev/doc/go1.14#compiler.
However, currently that dynamic check applies to explicit arithmetic like p = unsafe.Pointer(uintptr(p) + offset), but it does not currently apply to unsafe.Add, which is effectively an oversight.
I am currently working to resolve that in #74431.
I think that then is close to giving you what you asked for just above with CheckedAdd, though it is opt in via a compiler flag, rather than opting in via a new API? Maybe that is OK given your opening comment has:
We would like to check bounds in special ("invariant") builds,
I'm curious if you could say more about the use cases for panicking on a non-heap object as suggested by your doc comment in CheckedAdd? (Maybe I haven't thought it through properly, but it seems it would be useful to handle for example a stack object, and verify at least that the resulting pointer after arithmetic is also to a possible stack address, rather than panicking when handed a stack address).
Comment From: RaduBerinde
Hi @RaduBerinde, FWIW, using -gcflags=all=-d=checkptr does already cause the runtime to dynamically check that unsafe pointer arithmetic on a heap object ends up resulting in a pointer that is in bounds for that starting object (or at least one of the unsafe.Pointer operands, if there are multiple).
Thank you very much! This is great - I wasn't aware it checks at the time the arithmetic is performed. We already run msan and race builds (though they are much reduced in the amount of testing because they're much slower). I might experiment with just checkptr to see if it's fast enough to run the full tests. In any case, this does address the ask.
Thanks for fixing the unsafe.Add limitation. I recall we did have a bug (in cockroachdb/pebble) where we were doing pointer arithmetic and ending outside of the object and we didn't see a crash at the time the arithmetic was performed (in the msan/race builds). We might have been using unsafe.Add in that case which would explain it (it's also possible it was a hard to hit condition).
I'm curious if you could say more about the use cases for panicking on a non-heap object as suggested by your doc comment in CheckedAdd?
It was just bad phrasing, it's really about ensuring the arithmetic stays within the same object.
Comment From: gopherbot
Change https://go.dev/cl/690075 mentions this issue: cmd/compile/internal/walk: instrument unsafe.Add for checkptr=1 runtime pointer validation