runtime: `notInHeapSlice` conversion to normal slice unsafe for heap bits

[prattmic]

runtime.heapBitsSlice constructs a slice of heap ptr/scalar bits as a notInHeapSlice.

Since these bits live beyond mspan.limit, the GC must not encounter any pointers to them. findObject will throw due to an out-of-bounds pointer.

notInHeapSlice will hide the pointer from the GC [1]. Unfortunately, heapBitsSlice immediately converts the notInHeapSlice to []uintptr, a standard slice type. The data field of this slice will be marked as a pointer and is thus visible to the GC.

Thus if this slice is live when the GC runs, findObject will throw when it sees the invalid pointer.

This slice is ultimately returned from runtime.(*mspan).heapBits. The callers of heapBits all keep the slice live for a pretty short period. So previously, I believe we got lucky because (a) we don't async preempt in the runtime, and (b) there were no explicit preemption points while the slice is live.

Unfortunately, https://go.dev/cl/750480 adds a preemption point at the clear in initHeapBits. Within Google, we've found cases of applications crashing due to a preemption at this point allowing the GC to discover the bad slice.

My current plan is to revert https://go.dev/cl/750480 (and follow-up CLs) to get the tree back into a stable state. After the revert, we should fix this improper use of notInHeapSlice.

Note that most other uses of notInHeapSlice also convert to a standard slice, but all other uses are pointers to objects from persistentalloc or direct mmap. findObject ignores such pointers, so their visibility to the GC is not problematic.

[1] I think https://cs.opensource.google/go/go/+/master:src/cmd/compile/internal/types/size.go;drc=7c42da1bc5369d7ba174328908ecc9172578f6d2;bpv=1;bpt=1;l=714 records *notInHeap as a scalar, though I'm actually not completely sure.

cc @randall77 @golang/runtime

[gopherbot]

Change https://go.dev/cl/757343 mentions this issue: Revert "runtime, cmd/compile: use preemptible memclr for large pointer-free clears"

[gopherbot]

Change https://go.dev/cl/757341 mentions this issue: Revert "cmd/compile: don't call memclrNoHeapPointersPreemptible from nosplit functions"

[gopherbot]

Change https://go.dev/cl/757342 mentions this issue: Revert "runtime: fix memclrNoHeapPointersPreemptible"

[mknyszek]

After some discussion with @prattmic, I have a slight preference to try and eliminate this footgun in the runtime entirely.

The root of the problem isn't just the heap bits or the notInHeapSlice conversion, but rather the fact that all pointers into inline span GC metadata (pointer/scalar bits, inline mark bits for Green Tea) are considered invalid by the GC because they exceed mspan.limit.

One option is we make the GC consider such pointers as valid. This means GODEBUG=invalidptr=1 (which is enabled by default) becomes slightly less useful. If poor use of unsafe results in a pointer pointing into the metadata part of a span, then you won't get a crash anymore. I think that's OK; we have other mechanisms to help people debug bad uses of unsafe.

An alternative is to try and manage the pointers into this metadata explicitly, and ensure there are no preemption points while this metadata is being accessed. I think it's doable and I am not opposed. My only concern is that it adds yet another new invariant to the runtime that may lead to a new tail of hard-to-understand runtime bugs in the long term. Though, perhaps this new invariant will be isolated enough that it'll be fine in practice.

https://go.dev/cl/758302 is a small CL that implements the first option.

[gopherbot]

Change https://go.dev/cl/758302 mentions this issue: runtime: ignore pointers to inline span metadata in the GC