runtime: append can write out of bounds during data races

[ikonst]

When concurrently appending to the same slice, it's easy to get it to memcpy past the end due to struct (slice in this case) copy being non-atomic, i.e. if cap is copied before arr, concurrent code can use old arr with a new cap.

My issue isn't with the non-atomicity of the copy, or with slices being unsafe for data races, and I'm not making a case for communicating by sharing memory ;) Rather, it's about the failure mode being not only data loss in the particular slice (to be expected in a data structure that's not thread-safe) but also spooky actions at a distance (random heap writes, "impossible" crashes).

In practical programs, concurrent appends may be inadvertent. Packages use slices casually, in ways the author might not think to advertise, and the user might not think to inspect. Suppose a debug logging mechanism using a global slice - a slice which is not even read by anyone except when debugging - and then suppose someone starts concurrently calling a function which uses the logging function. This could result in a heap corruption that would be difficult to track down.

There might be practical solutions, e.g.

• make it so that arr is copied before cap
• store the capacity in the head of the arr buffer rather than in cap

(Apologies if an obvious dup, I couldn't find one.)

[gabyhelp]

Related Issues

• runtime/race: Data race in append() missed by the race detector #22762 (closed)
• runtime: Append consecutive slices, same underlying array #17530 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

In general data races will do bad things. (I sympathize: we are currently struggling to find the cause of memory corruption in gopls, presumably a data race, though we don't know for sure.) Fortunately, if the race can be reproduced by a test, then the cause is readily identified by running go test -race to enable the race detector, which emits an explanation of the object and participants of the race.

There has been some research into the possibility of changing the compiler and runtime so that multiword data structures (slices, strings, and interfaces) are accessed using only 2-word load/store operations. However in the case of slices, this does require reordering the fields (a breaking change to all assembly code and to many uses of unsafe.Pointer) so that the cap and data fields are updated atomically. (The len field is less critical since it is safe to use if 0 <= len <= cap.) @dr2chase knows more.

Short of the latter project, I doubt there's much that can be done in the normal case to limit the "blast radius" of a data race without hurting the performance of well-behaved code.

[thepudds]

Hi @ikonst, just a quick comment to link to a classic blog post by Russ on the topic of data races at https://research.swtch.com/gorace, which includes:

In the current Go implementations, though, there are two ways to break through these safety mechanisms. [...] The second, less direct way is to use a data race in a multithreaded program.

and:

The race is fundamentally caused by being able to observe partial updates to Go's multiword values (slices, interfaces, and strings): the updates are not atomic.

[thepudds]

(Also, whatever the resolution here, I suspect most likely it would not need to go through the proposal process, so removing that label, at least for now).

[ikonst]

@thepudds Thank you for the blog post and in particular the highlights. Clearly I'm not on to anything new here :)

It's a bit unfortunate that 15 years went by without much progress on this issue. Russ ends his post with "[...] Once the Go implementations are more mature...", so in those terms Go should be at the very least attending high school at this point.

As to the solution, Russ doesn't mention "atomic two-word load/store" in his post. I didn't even know this was a thing.

He does mention the solution of having a pointer to the array and cap. (I'm not sure why you need the extra arr indirection, rather than allocating an array with an extra word at its head.) What precludes us from implementing that solution is the fallout from impacting users of unsafe.Pointer and/or undocumented APIs (aka members of the hall of shame)?

[mknyszek]

CC @dr2chase since you did some work related to this.

[dr2chase]

@ikonst sorry for the late reply, the "problem" with fixing the problem is a combination of breaking unsafe code, and generally slowing down code whose struct layouts are made worse by the change in alignment for slices, strings, and interfaces. Allowing the compiler to (by default, unless noted) reorder struct fields to reduce alignment padding (and crowd pointers towards the beginning) removes most of that cost, but that is another large change. We're (I'm) working on it, slowly -- structs.HostLayout is intended to be the signal that a structure cannot be reordered, and there are CLs queued up that will automatically insert that for cgo-generated structs.

It's somewhat back-burner because other things are always more urgent, but I hope to make progress in the next few releases.