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

runtime: non-atomic pointer writes in memclr/memmove #13160

Closed
dvyukov opened this issue Nov 5, 2015 · 5 comments
Closed

runtime: non-atomic pointer writes in memclr/memmove #13160

dvyukov opened this issue Nov 5, 2015 · 5 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented Nov 5, 2015

The following program crashes when run with GODEBUG=invalidptr=1 GOARCH=386:

package main

import (
    "math/rand"
    "runtime"
)

func main() {
    P := 2 * runtime.GOMAXPROCS(0)
    ptrs := make([][]*int, P)
    for i := 0; i < P; i++ {
        i := i
        go func() {
            r := rand.New(rand.NewSource(int64(i)))
            var sink []byte
            p := make([]*int, 4)
            z := make([]*int, 4)
            ptrs[i] = p
            for {
                if r.Intn(20) != 0 {
                    a := p[:1]
                    a[0] = new(int)
                    for j := range a {
                        a[j] = nil
                    }
                    for j := range p {
                        p[j] = new(int)
                    }
                    copy(p[:3], z)
                    for j := range p {
                        p[j] = new(int)
                    }
                    copy(p[1:4], z)
                    for j := range p {
                        p[j] = new(int)
                        copy(p[j:j+1], z)
                    }
                } else {
                    sink = make([]byte, 8<<10)
                }
            }
            _ = sink
        }()
    }
    select {}
}
GODEBUG=invalidptr=1 GOARCH=386 go run memclr.go

runtime: pointer 0x18d90000 to unallocated spanidx=0x548 span.start=0x185d2000 span.limit=0x185d4000 span.state=3
runtime: found in object at *(0x18456000+0x0)
object=0x18456000 k=0xc22b s.start*_PageSize=0x18456000 s.limit=0x18458000 s.sizeclass=2 s.elemsize=16
 *(object+0) = 0x0 <==
 *(object+4) = 0x0
 *(object+8) = 0x18d9b354
 *(object+12) = 0x18d9b3b0
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

The problem is that memclr clears 4-byte regions on 386 with 2 separate 2-byte stores:

_3or4:
    MOVW    AX, (DI)
    MOVW    AX, -2(DI)(BX*1)

This exposes corrupted pointer to GC.

Not sure whether it can crash without invalidptr, I would not exclude such possibility. But it's clearly bad and can lead to false retention. Also it makes data race failure modes significantly worse, introducing exploitation possibilities like use-after-free and easy controlled overwrite.

There is similar issue in amd64p32 memclr which clears pointers with byte stores:

TEXT runtime·memclr(SB),NOSPLIT,$0-8
    MOVL    ptr+0(FP), DI
    MOVL    n+4(FP), CX
    MOVQ    CX, BX
    ANDQ    $3, BX
    SHRQ    $2, CX
    MOVQ    $0, AX
    CLD
    REP
    STOSL
    MOVQ    BX, CX
    REP
    STOSB
    RET

Similar issue in 386 memmove which moves pointer with 2 stores:

move_3or4:
    MOVW    (SI), AX
    MOVW    -2(SI)(BX*1), CX
    MOVW    AX, (DI)
    MOVW    CX, -2(DI)(BX*1)
    RET

Similar issue in plan9/386 memclr:

_1or2:
    MOVB    AX, (DI)
    MOVB    AX, -1(DI)(BX*1)

Similar issue in amd64 memclr:

_5through8:
    MOVL    AX, (DI)
    MOVL    AX, -4(DI)(BX*1)

Did not look at arm/ppc. I guess we just need to go through all of them one-by-one.
@rsc @randall77 @RLH @aclements @dr2chase @davecheney @minux @0intro @4ad

@dvyukov dvyukov added this to the Go1.6 milestone Nov 5, 2015
@dvyukov
Copy link
Member Author

dvyukov commented Nov 5, 2015

@ality

@aclements
Copy link
Member

Amusingly arm/ppc are probably okay because we tracked this exact issue down in #12552. At the time I thought the other platforms were okay, but you're definitely right about the tails.

@randall77 randall77 self-assigned this Nov 5, 2015
@randall77
Copy link
Contributor

I checked all the memclr & memmove code, CL out for review. It was just x86, the other architectures look ok to me.

The only other place we do byte-at-a-time copy is in the reflect.callX functions. Both the source (a newly allocated chunk of memory) and the destination (the local stack) should be ok for non-atomic pointer copy.

@gopherbot
Copy link

CL https://golang.org/cl/16668 mentions this issue.

@rsc rsc modified the milestones: Go1.5.2, Go1.6 Nov 6, 2015
@rsc
Copy link
Contributor

rsc commented Nov 6, 2015

This might be worth fixing for Go 1.5.2, if we release a Go 1.5.2.

aclements pushed a commit that referenced this issue Nov 13, 2015
Make sure that we're moving or zeroing pointers atomically.
Anything that is a multiple of pointer size and at least
pointer aligned might have pointers in it.  All the code looks
ok except for the 1-pointer-sized moves.

Fixes #13160
Update #12552

Change-Id: Ib97d9b918fa9f4cc5c56c67ed90255b7fdfb7b45
Reviewed-on: https://go-review.googlesource.com/16668
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/16910
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc rsc changed the title runtime: crash due to non-atomic pointer stores in memclr/memmove runtime: non-atomic pointer stores in memclr/memmove Nov 13, 2015
@rsc rsc changed the title runtime: non-atomic pointer stores in memclr/memmove runtime: non-atomic pointer writes in memclr/memmove Nov 13, 2015
@golang golang locked and limited conversation to collaborators Nov 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants