Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unsafe use of pointers in sample code #16

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

zolstein
Copy link

@zolstein zolstein commented Jul 5, 2022

benchmark_marshaler_test.go and its copy in the README demonstrated
unsafe conversion of uintptr -> unsafe.Pointer. Fixing this will
hopefully reduce the likelihood that users will write unsafe code.


I noticed the unsafety while reading this blog post, which says:

In effect, once we’ve converted an unsafe.Pointer to uintptr, we cannot safely convert it back to unsafe.Pointer, with the exception of one special case:

If p points into an allocated object, it can be advanced through the object by conversion to uintptr, addition of an offset, and conversion back to Pointer.

In order to perform this pointer arithmetic iteration logic safely, we must perform the type conversions and pointer arithmetic all at once

As I understand, the reason that the old version was unsafe and this is not is that Go reserves the right for the garbage-collector to move objects, which may invalidate the uintptr pointing to the struct field before the field can be accessed. However, by doing the conversion to a uintptr, the pointer arithmetic, and the conversation back to an unsafe.Pointer in one statement, the value retains its pointer semantics through the whole operation and is guaranteed to be updated if the underlying object is moved.

(I don't believe the current GC actually moves objects, so probably the old version would always work... unless / until a new version of Go introduces this, at which point something like this could cause very rare breakages.)

Even though this code is meant as a benchmark and sample use-case, since users will try to copy this code to write similar code, it's probably best to demonstrate good practices that won't break in the future.

go vet validates that the old version was unsafe and the new version is not. Running the benchmark with the old and new versions of the code seemed to show equivalent performance and no additional allocations.

benchmark_marshaler_test.go and its copy in the README demonstrated
unsafe conversion of uintptr -> unsafe.Pointer. Fixing this will
hopefully reduce the likelihood that users will write unsafe code.
@goccy
Copy link
Owner

goccy commented Jul 6, 2022

You are right. Thank you for the contribution !

@goccy goccy merged commit 62880e4 into goccy:master Jul 6, 2022
@zolstein zolstein deleted the unsafe-ptr branch July 6, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants