unsafe: allow conversion of uintptr to unsafe.Pointer when it points to non-Go memory

[bcmills]

Background

The documentation for unsafe.Pointer currently lists six valid conversion patterns:

1. “Conversion of a *T1 to Pointer to *T2 … [p]rovided that T2 is no larger than T1 and that the two share an equivalent memory layout.”

2. “Conversion of a Pointer to a uintptr (but not back to Pointer).”

3. “Conversion of a Pointer to a uintptr and back, with arithmetic … in the same expression, with only the intervening arithmetic between them.”

4. “Conversion of a Pointer to a uintptr when calling syscall.Syscall … [or] in the argument list of a call to a function implemented in assembly.” (Compare proposal: unsafe: clarify unsafe.Pointer rules for package syscall #34684.)

5. “Conversion of the result of reflect.Value.Pointer or reflect.Value.UnsafeAddr from uintptr to Pointer … immediately after making the call, in the same expression.”

6. “Conversion of a reflect.SliceHeader or reflect.StringHeader Data field to or from Pointer … when interpreting the content of an actual slice or string value.”

The unsafeptr check provided by cmd/vet warns about uses that do not follow the above patterns. However, it currently flags many violations in x/sys/unix (see #41205) when addresses returned by system calls such as mmap are converted to Go pointers, and in code generated by ebitengine/purego (see #56487) when hard-coded addresses provided by the operating system or linker are converted to Go pointers. In both cases, the program is attempting to create Go pointers that refer to known-valid addresses that are not managed by the Go runtime; notably, the compiler's -d=checkptr mode does not flag them as invalid at run-time.

Ideally, the unsafe.Pointer documentation, the unsafeptr check in cmd/vet, the -d=checkptr mode in cmd/compile, and the real-world usage in syscall, x/sys, and similar low-level libraries should all agree on what is valid. This proposal aims to narrow that gap.

Proposal

I propose that we add another allowed case in the unsafe.Pointer documentation:

(7) Conversion of a uintptr to Pointer when the address is allocated outside of Go.

A uintptr containing a valid memory address allocated outside of Go
(such as by a system call) may be converted to Pointer.
The address must remain valid for as long as any Go pointer (of type Pointer
or any other pointer type) refers to it.
The address must remain unavailable to the Go runtime (for example, due to an
allocation using cgo or syscall.Mmap) for as long as any Go pointer
(of type Pointer or any other pointer type) refers to it.
[edited per https://github.com/golang/go/issues/58625#issuecomment-1440188404\]

The uintptr constant 0 may be converted to Pointer.
The resulting Pointer has the value nil.

addr, _, err := syscall.Syscall(…)
…
p := unsafe.Pointer(addr)

The unsafeptr check in cmd/vet would be changed to allow the new case, resolving the warnings in x/sys and ebitengine/purego.

The compiler's -d=checkptr mode would check conversions from uintptr to unsafe.Pointer. The conversion may throw at run-time if:

• The uintptr refers to an address managed by Go that is not explicitly pinned (runtime: provide Pinner API for object pinning #46787), or
• the uintptr refers to an invalid (unmapped) nonzero address.
  [edit: per unsafe: allow conversion of uintptr to unsafe.Pointer when it points to non-Go memory #58625 (comment), unmapped addresses should be allowed as long as they cannot become Go addresses]

Alternatives

The vet warning can be worked around today by relying on a liberal reading of the “equivalent memory layout” rule, rewriting

var addr uintptr = …
p := unsafe.Pointer(addr)

(https://go.dev/play/p/ZWZxv6URqTW) as

var addr uintptr = …
p := *(*unsafe.Pointer)(unsafe.Pointer(&addr))

(https://go.dev/play/p/Wh5f8k0_yyR), on the theory that the memory layout of a uintptr must be in some sense “equivalent” to the memory layout of unsafe.Pointer. However, I believe that such a rewrite does not capture the intent of the code as accurately, and should not be necessary.

(CC @golang/runtime)

[prattmic]

Overall, this looks good to me. A few minor questions:

The address must remain valid for as long as any Go pointer (of type Pointer or any other pointer type) refers to it.

Do we need this requirement? I don't think the GC will follow non-Go pointers, so I think this should be fine as long the application doesn't dereference the pointer when it is invalid. (I am imagining a library that temporarily unmaps (or more likely, protects PROT_NONE) pages during transient periods when they should not be accessed).

The compiler's -d=checkptr mode would check conversions from uintptr to unsafe.Pointer. The conversion may throw at run-time if: the uintptr refers to an invalid (unmapped) nonzero address.

How do we implement this? Add an unconditional dereference of the pointer? Does a PROT_NONE-protected page count as "valid"? If so, a dereference isn't sufficient, you'd need to either try to map over the page, or parse /proc/self/maps.

[bcmills]

Do we need this requirement? I don't think the GC will follow non-Go pointers

I think the bad pointer in Go heap check will trigger if an unsafe.Pointer variable whose storage is itself in Go memory contains a value that is too bogus (like maybe too close to 0, or formerly-but-no-longer part of the Go heap)?

Plus, if the non-Go allocator does actually unmap the address (instead of mapping it PROT_NONE), then it's possible the Go allocator will later make use of that address while the pointer still exists.

I would be ok with weakening the requirement to “must remain mapped”, but I'm not sure that I want to try to define “mapped” portably. 😅

[bcmills]

How do we implement this?

Heavy emphasis on the “may”?

I was thinking we would call runtime.findObject and let it do its thing: if it calls badPointer so be it, and if it reports a nonzero base we fail with a checkptr error.

[ianlancetaylor]

At first glance updating vet for this means simply permitting all conversions from uintptr to unsafe.Pointer, because in general vet has no idea whether the value is valid. Can we do better?

[ianlancetaylor]

"This address must remain valid" seems both too strong and not strong enough. What actually matters is that the address is never allocated by the Go runtime. I'm not sure how to phrase that, though.

[bcmills]

At first glance updating vet for this means simply permitting all conversions from uintptr to unsafe.Pointer

I agree that's the simplest change to make to it. Part of the intent is to shift the burden of enforcement from a relatively noisy static check (unsafeptr) to a more precise dynamic one (checkptr, which is enabled by default under the race detector).

It is possible in theory for vet to continue to notice and flag cases where the uintptr is always derived from a pointer to Go memory, but I'm not sure it would be worth the effort to implement given that checkptr could detect those cases with very high precision.

Also, the large number of vet warnings in x/sys suggests that the existing vet check is probably not widely used today. I'm not sure that we would lose much in gutting it.

[bcmills]

What actually matters is that the address is never allocated by the Go runtime. I'm not sure how to phrase that, though.

Maybe:
“A uintptr containing a memory address allocated outside of Go
may be converted to Pointer. The address must remain unavailable
to the Go runtime (for example, due to an allocation using cgo or
syscall.Mmap) for as long as any Go pointer (of type Pointer
or any other pointer type) refers to it.”

[mknyszek]

The address must remain unavailable to the Go runtime (for example, due to an allocation using cgo or syscall.Mmap) for as long as any Go pointer (of type Pointer or any other pointer type) refers to it.

Tying these two things together worries me. Having a program be incorrect if it leaves behind a pointer it never dereferences to some memory region that was unmapped and remapped by the Go runtime feels too subtle to me. I'm also not sure how to implement a dynamic check for this case since there's no way the runtime could identify that a pointer just sitting there previously referred to non-Go memory.

This is something of an insight from the arena proposal: it's really not safe to just unmap memory regions that Go pointers reference. The arena implementation waits until the GC can't find a Go pointer to the region before allowing that address space to be reused.

One way we can make this OK is to support GC-managed memory regions, where the GC will let you know (via a finalizer or similar mechanism) that it's really OK to unmap a region. (Side note: this is actually fairly easy to implement today, if we require/guarantee that memory regions are aligned to 8 KiB, and are at least 8 KiB in size. Unfortunately our most popular platforms all have 4 KiB physical pages...)

[bcmills]

Having a program be incorrect if it leaves behind a pointer it never dereferences to some memory region that was unmapped and remapped by the Go runtime feels too subtle to me.

That's why it's unsafe! 😅

I'm also not sure how to implement a dynamic check for this case since there's no way the runtime could identify that a pointer just sitting there previously referred to non-Go memory.

If the GC rate is high enough, eventually the pointer will be unallocated when the GC scans it and trip the badPointer check, no?

This is something of an insight from the arena proposal: it's really not safe to just unmap memory regions that Go pointers reference.

Agreed, but I expect that in the vast majority of cases, the addresses converted from uintptr to unsafe.Pointer will never be unmapped and it will be a non-issue.

On the other hand, to the extent that it is a problem under this proposal, it is already a problem under cgo in general today: suppose that a C function allocates a memory region using mmap, passes that pointer into a C-exported Go function (perhaps as type void* a.k.a. unsafe.Pointer), and unmaps it after the Go function returns. That's exactly the same problem without involving uintptr conversions, no? If the runtime happens to acquire that address in a subsequent allocation, any lingering pointers may trigger the badPointer check during the next GC cycle.

[bcmills]

(CC @dominikh @timothy-king)