Skip to content

Commit

Permalink
internal/cache: fix memory corruption when CGO_ENABLED=0
Browse files Browse the repository at this point in the history
If cgo is disabled, `manual.New` allocates memory using
`make([]byte,...)`. The cache was using this memory as if it was
manually allocated, and casting it to Go structs and then storing Go
pointers inside the structs. But a Go pointer stored inside of a
`[]byte` is invisible to the Go GC, so the GC was merrily reclaiming the
memory which would lead to all sorts of badness in the cache, often
resulting in segfaults or violations of refcount invariants.

Now, when cgo is disabled, the `entry` and `Value` structs are directly
allocated using the Go allocator. The cache code mostly supported this
mode of operation already when the `invariants` tag. The difference
between disabling cgo and enabling invariants is that the latter also
has the effect of adding the finalizers uses for leak detection.

Fixes #1010
  • Loading branch information
petermattis committed Nov 25, 2020
1 parent e325d9d commit c2b05f1
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 1 deletion.
4 changes: 4 additions & 0 deletions .travis.yml
Expand Up @@ -30,6 +30,10 @@ matrix:
go: 1.15.x
os: linux
script: make test TAGS=
- name: "go1.15.x-linux-no-cgo"
go: 1.15.x
os: linux
script: CGO_ENABLED=0 make test TAGS=
- name: "go1.15.x-darwin"
go: 1.15.x
os: osx
Expand Down
9 changes: 9 additions & 0 deletions internal/cache/cgo_disabled.go
@@ -0,0 +1,9 @@
// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
// of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.

// +build !cgo

package cache

const cgoEnabled = false
9 changes: 9 additions & 0 deletions internal/cache/cgo_enabled.go
@@ -0,0 +1,9 @@
// Copyright 2020 The LevelDB-Go and Pebble Authors. All rights reserved. Use
// of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.

// +build cgo

package cache

const cgoEnabled = true
6 changes: 5 additions & 1 deletion internal/cache/entry_normal.go
Expand Up @@ -21,7 +21,11 @@ const (
// Avoid using runtime.SetFinalizer in race builds as finalizers tickle a bug
// in the Go race detector in go1.15 and earlier versions. This requires that
// entries are Go allocated rather than manually allocated.
entriesGoAllocated = invariants.RaceEnabled
//
// If cgo is disabled we need to allocate the entries using the Go allocator
// and is violates the Go GC rules to put Go pointers (such as the entry
// pointer fields) into untyped memory (i.e. a []byte).
entriesGoAllocated = invariants.RaceEnabled || !cgoEnabled
)

var entryAllocPool = sync.Pool{
Expand Down
13 changes: 13 additions & 0 deletions internal/cache/value_normal.go
Expand Up @@ -18,6 +18,15 @@ func newValue(n int) *Value {
if n == 0 {
return nil
}

if !cgoEnabled {
// If Cgo is disabled then all memory is allocated from the Go heap and we
// can't play the trick below to combine the Value and buffer allocation.
v := &Value{buf: make([]byte, n)}
v.ref.init(1)
return v
}

// When we're not performing leak detection, the lifetime of the returned
// Value is exactly the lifetime of the backing buffer and we can manually
// allocate both.
Expand All @@ -34,6 +43,10 @@ func newValue(n int) *Value {
}

func (v *Value) free() {
if !cgoEnabled {
return
}

// When we're not performing leak detection, the Value and buffer were
// allocated contiguously.
n := valueSize + cap(v.buf)
Expand Down

0 comments on commit c2b05f1

Please sign in to comment.