From 5c1052b6bb2658208f1afaf423aeac98f30235c1 Mon Sep 17 00:00:00 2001 From: Zach Koopmans Date: Wed, 14 Apr 2021 11:27:05 -0700 Subject: [syserror] Remove syserror from go_marshal PiperOrigin-RevId: 368470656 --- pkg/gohacks/BUILD | 10 ++- pkg/gohacks/gohacks_test.go | 97 ++++++++++++++++++++++ pkg/gohacks/gohacks_unsafe.go | 14 ++++ pkg/marshal/marshal.go | 3 + pkg/safecopy/safecopy_test.go | 62 -------------- pkg/sentry/time/BUILD | 1 + pkg/sync/BUILD | 1 + pkg/sync/generic_seqatomic_unsafe.go | 3 +- pkg/sync/runtime_unsafe.go | 14 ---- pkg/sync/seqatomictest/BUILD | 1 + tools/go_marshal/defs.bzl | 1 - tools/go_marshal/gomarshal/generator.go | 2 - .../generator_interfaces_array_newtype.go | 8 +- .../generator_interfaces_primitive_newtype.go | 24 +++--- .../gomarshal/generator_interfaces_struct.go | 33 ++++---- 15 files changed, 157 insertions(+), 117 deletions(-) create mode 100644 pkg/gohacks/gohacks_test.go diff --git a/pkg/gohacks/BUILD b/pkg/gohacks/BUILD index 35683fe98..b4e05f922 100644 --- a/pkg/gohacks/BUILD +++ b/pkg/gohacks/BUILD @@ -1,4 +1,4 @@ -load("//tools:defs.bzl", "go_library") +load("//tools:defs.bzl", "go_library", "go_test") package(licenses = ["notice"]) @@ -10,3 +10,11 @@ go_library( stateify = False, visibility = ["//:sandbox"], ) + +go_test( + name = "gohacks_test", + size = "small", + srcs = ["gohacks_test.go"], + library = ":gohacks", + deps = ["@org_golang_x_sys//unix:go_default_library"], +) diff --git a/pkg/gohacks/gohacks_test.go b/pkg/gohacks/gohacks_test.go new file mode 100644 index 000000000..e18c8abc7 --- /dev/null +++ b/pkg/gohacks/gohacks_test.go @@ -0,0 +1,97 @@ +// Copyright 2021 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gohacks + +import ( + "io/ioutil" + "math/rand" + "os" + "runtime/debug" + "testing" + + "golang.org/x/sys/unix" +) + +func randBuf(size int) []byte { + b := make([]byte, size) + for i := range b { + b[i] = byte(rand.Intn(256)) + } + return b +} + +// Size of a page in bytes. Cloned from hostarch.PageSize to avoid a circular +// dependency. +const pageSize = 4096 + +func testCopy(dst, src []byte) (panicked bool) { + defer func() { + if r := recover(); r != nil { + panicked = true + } + }() + debug.SetPanicOnFault(true) + copy(dst, src) + return panicked +} + +func TestSegVOnMemmove(t *testing.T) { + // Test that SIGSEGVs received by runtime.memmove when *not* doing + // CopyIn or CopyOut work gets propagated to the runtime. + const bufLen = pageSize + a, err := unix.Mmap(-1, 0, bufLen, unix.PROT_NONE, unix.MAP_ANON|unix.MAP_PRIVATE) + if err != nil { + t.Fatalf("Mmap failed: %v", err) + + } + defer unix.Munmap(a) + b := randBuf(bufLen) + + if !testCopy(b, a) { + t.Fatalf("testCopy didn't panic when it should have") + } + + if !testCopy(a, b) { + t.Fatalf("testCopy didn't panic when it should have") + } +} + +func TestSigbusOnMemmove(t *testing.T) { + // Test that SIGBUS received by runtime.memmove when *not* doing + // CopyIn or CopyOut work gets propagated to the runtime. + const bufLen = pageSize + f, err := ioutil.TempFile("", "sigbus_test") + if err != nil { + t.Fatalf("TempFile failed: %v", err) + } + os.Remove(f.Name()) + defer f.Close() + + a, err := unix.Mmap(int(f.Fd()), 0, bufLen, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED) + if err != nil { + t.Fatalf("Mmap failed: %v", err) + + } + defer unix.Munmap(a) + b := randBuf(bufLen) + + if !testCopy(b, a) { + t.Fatalf("testCopy didn't panic when it should have") + } + + if !testCopy(a, b) { + t.Fatalf("testCopy didn't panic when it should have") + } +} diff --git a/pkg/gohacks/gohacks_unsafe.go b/pkg/gohacks/gohacks_unsafe.go index 10bbb1f58..374aac2b4 100644 --- a/pkg/gohacks/gohacks_unsafe.go +++ b/pkg/gohacks/gohacks_unsafe.go @@ -75,3 +75,17 @@ func StringFromImmutableBytes(bs []byte) string { // strings.Builder.String(). return *(*string)(unsafe.Pointer(&bs)) } + +// Note that go:linkname silently doesn't work if the local name is exported, +// necessitating an indirection for exported functions. + +// Memmove is runtime.memmove, exported for SeqAtomicLoad/SeqAtomicTryLoad. +// +//go:nosplit +func Memmove(to, from unsafe.Pointer, n uintptr) { + memmove(to, from, n) +} + +//go:linkname memmove runtime.memmove +//go:noescape +func memmove(to, from unsafe.Pointer, n uintptr) diff --git a/pkg/marshal/marshal.go b/pkg/marshal/marshal.go index eb036feae..7da450ce8 100644 --- a/pkg/marshal/marshal.go +++ b/pkg/marshal/marshal.go @@ -166,6 +166,9 @@ type Marshallable interface { // %s is the first argument to the slice clause. This directive is not supported // for newtypes on arrays. // +// Note: Partial copies are not supported for Slice API UnmarshalUnsafe and +// MarshalUnsafe. +// // The slice clause also takes an optional second argument, which must be the // value "inner": // diff --git a/pkg/safecopy/safecopy_test.go b/pkg/safecopy/safecopy_test.go index 611f36253..55743e69c 100644 --- a/pkg/safecopy/safecopy_test.go +++ b/pkg/safecopy/safecopy_test.go @@ -19,8 +19,6 @@ import ( "fmt" "io/ioutil" "math/rand" - "os" - "runtime/debug" "testing" "unsafe" @@ -568,63 +566,3 @@ func TestCompareAndSwapUint32BusError(t *testing.T) { } }) } - -func testCopy(dst, src []byte) (panicked bool) { - defer func() { - if r := recover(); r != nil { - panicked = true - } - }() - debug.SetPanicOnFault(true) - copy(dst, src) - return -} - -func TestSegVOnMemmove(t *testing.T) { - // Test that SIGSEGVs received by runtime.memmove when *not* doing - // CopyIn or CopyOut work gets propagated to the runtime. - const bufLen = pageSize - a, err := unix.Mmap(-1, 0, bufLen, unix.PROT_NONE, unix.MAP_ANON|unix.MAP_PRIVATE) - if err != nil { - t.Fatalf("Mmap failed: %v", err) - - } - defer unix.Munmap(a) - b := randBuf(bufLen) - - if !testCopy(b, a) { - t.Fatalf("testCopy didn't panic when it should have") - } - - if !testCopy(a, b) { - t.Fatalf("testCopy didn't panic when it should have") - } -} - -func TestSigbusOnMemmove(t *testing.T) { - // Test that SIGBUS received by runtime.memmove when *not* doing - // CopyIn or CopyOut work gets propagated to the runtime. - const bufLen = pageSize - f, err := ioutil.TempFile("", "sigbus_test") - if err != nil { - t.Fatalf("TempFile failed: %v", err) - } - os.Remove(f.Name()) - defer f.Close() - - a, err := unix.Mmap(int(f.Fd()), 0, bufLen, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED) - if err != nil { - t.Fatalf("Mmap failed: %v", err) - - } - defer unix.Munmap(a) - b := randBuf(bufLen) - - if !testCopy(b, a) { - t.Fatalf("testCopy didn't panic when it should have") - } - - if !testCopy(a, b) { - t.Fatalf("testCopy didn't panic when it should have") - } -} diff --git a/pkg/sentry/time/BUILD b/pkg/sentry/time/BUILD index 87d8687ce..1f617ca8f 100644 --- a/pkg/sentry/time/BUILD +++ b/pkg/sentry/time/BUILD @@ -32,6 +32,7 @@ go_library( ], visibility = ["//:sandbox"], deps = [ + "//pkg/gohacks", "//pkg/log", "//pkg/metric", "//pkg/sync", diff --git a/pkg/sync/BUILD b/pkg/sync/BUILD index b2c5229e7..8b3a11c64 100644 --- a/pkg/sync/BUILD +++ b/pkg/sync/BUILD @@ -43,6 +43,7 @@ go_template( ], deps = [ ":sync", + "//pkg/gohacks", ], ) diff --git a/pkg/sync/generic_seqatomic_unsafe.go b/pkg/sync/generic_seqatomic_unsafe.go index 82b676abf..9578c9c52 100644 --- a/pkg/sync/generic_seqatomic_unsafe.go +++ b/pkg/sync/generic_seqatomic_unsafe.go @@ -10,6 +10,7 @@ package seqatomic import ( "unsafe" + "gvisor.dev/gvisor/pkg/gohacks" "gvisor.dev/gvisor/pkg/sync" ) @@ -39,7 +40,7 @@ func SeqAtomicTryLoad(seq *sync.SeqCount, epoch sync.SeqCountEpoch, ptr *Value) // runtime.RaceDisable() doesn't actually stop the race detector, so it // can't help us here. Instead, call runtime.memmove directly, which is // not instrumented by the race detector. - sync.Memmove(unsafe.Pointer(&val), unsafe.Pointer(ptr), unsafe.Sizeof(val)) + gohacks.Memmove(unsafe.Pointer(&val), unsafe.Pointer(ptr), unsafe.Sizeof(val)) } else { // This is ~40% faster for short reads than going through memmove. val = *ptr diff --git a/pkg/sync/runtime_unsafe.go b/pkg/sync/runtime_unsafe.go index 158985709..39c766331 100644 --- a/pkg/sync/runtime_unsafe.go +++ b/pkg/sync/runtime_unsafe.go @@ -17,20 +17,6 @@ import ( "unsafe" ) -// Note that go:linkname silently doesn't work if the local name is exported, -// necessitating an indirection for exported functions. - -// Memmove is runtime.memmove, exported for SeqAtomicLoad/SeqAtomicTryLoad. -// -//go:nosplit -func Memmove(to, from unsafe.Pointer, n uintptr) { - memmove(to, from, n) -} - -//go:linkname memmove runtime.memmove -//go:noescape -func memmove(to, from unsafe.Pointer, n uintptr) - // Gopark is runtime.gopark. Gopark calls unlockf(pointer to runtime.g, lock); // if unlockf returns true, Gopark blocks until Goready(pointer to runtime.g) // is called. unlockf and its callees must be nosplit and norace, since stack diff --git a/pkg/sync/seqatomictest/BUILD b/pkg/sync/seqatomictest/BUILD index 5c38c783e..5f9164117 100644 --- a/pkg/sync/seqatomictest/BUILD +++ b/pkg/sync/seqatomictest/BUILD @@ -18,6 +18,7 @@ go_library( name = "seqatomic", srcs = ["seqatomic_int_unsafe.go"], deps = [ + "//pkg/gohacks", "//pkg/sync", ], ) diff --git a/tools/go_marshal/defs.bzl b/tools/go_marshal/defs.bzl index e23901815..9f620cb76 100644 --- a/tools/go_marshal/defs.bzl +++ b/tools/go_marshal/defs.bzl @@ -57,7 +57,6 @@ go_marshal = rule( # marshal_deps are the dependencies requied by generated code. marshal_deps = [ "//pkg/gohacks", - "//pkg/safecopy", "//pkg/hostarch", "//pkg/marshal", ] diff --git a/tools/go_marshal/gomarshal/generator.go b/tools/go_marshal/gomarshal/generator.go index 0e2d752cb..00961c90d 100644 --- a/tools/go_marshal/gomarshal/generator.go +++ b/tools/go_marshal/gomarshal/generator.go @@ -112,10 +112,8 @@ func NewGenerator(srcs []string, out, outTest, outTestUnconditional, pkg string, g.imports.add("runtime") g.imports.add("unsafe") g.imports.add("gvisor.dev/gvisor/pkg/gohacks") - g.imports.add("gvisor.dev/gvisor/pkg/safecopy") g.imports.add("gvisor.dev/gvisor/pkg/hostarch") g.imports.add("gvisor.dev/gvisor/pkg/marshal") - return &g, nil } diff --git a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go index 32afece2e..e8ce6e366 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_array_newtype.go @@ -33,13 +33,13 @@ func (g *interfaceGenerator) validateArrayNewtype(n *ast.Ident, a *ast.ArrayType } func (g *interfaceGenerator) emitMarshallableForArrayNewtype(n *ast.Ident, a *ast.ArrayType, elt *ast.Ident) { + g.recordUsedImport("gohacks") + g.recordUsedImport("hostarch") g.recordUsedImport("io") g.recordUsedImport("marshal") g.recordUsedImport("reflect") g.recordUsedImport("runtime") - g.recordUsedImport("safecopy") g.recordUsedImport("unsafe") - g.recordUsedImport("hostarch") lenExpr := g.arrayLenExpr(a) @@ -89,14 +89,14 @@ func (g *interfaceGenerator) emitMarshallableForArrayNewtype(n *ast.Ident, a *as g.emit("// MarshalUnsafe implements marshal.Marshallable.MarshalUnsafe.\n") g.emit("func (%s *%s) MarshalUnsafe(dst []byte) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("safecopy.CopyIn(dst, unsafe.Pointer(%s))\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&%s[0]), uintptr(len(dst)))\n", g.r) }) g.emit("}\n\n") g.emit("// UnmarshalUnsafe implements marshal.Marshallable.UnmarshalUnsafe.\n") g.emit("func (%s *%s) UnmarshalUnsafe(src []byte) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("safecopy.CopyOut(unsafe.Pointer(%s), src)\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(len(src)))\n", g.r) }) g.emit("}\n\n") diff --git a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go index 05f0e0db4..19aa5994a 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_primitive_newtype.go @@ -95,13 +95,13 @@ func (g *interfaceGenerator) validatePrimitiveNewtype(t *ast.Ident) { // newtypes are always packed, so we can omit the various fallbacks required for // non-packed structs. func (g *interfaceGenerator) emitMarshallableForPrimitiveNewtype(nt *ast.Ident) { + g.recordUsedImport("gohacks") + g.recordUsedImport("hostarch") g.recordUsedImport("io") g.recordUsedImport("marshal") g.recordUsedImport("reflect") g.recordUsedImport("runtime") - g.recordUsedImport("safecopy") g.recordUsedImport("unsafe") - g.recordUsedImport("hostarch") g.emit("// SizeBytes implements marshal.Marshallable.SizeBytes.\n") g.emit("//go:nosplit\n") @@ -141,14 +141,14 @@ func (g *interfaceGenerator) emitMarshallableForPrimitiveNewtype(nt *ast.Ident) g.emit("// MarshalUnsafe implements marshal.Marshallable.MarshalUnsafe.\n") g.emit("func (%s *%s) MarshalUnsafe(dst []byte) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("safecopy.CopyIn(dst, unsafe.Pointer(%s))\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(%s), uintptr(len(dst)))\n", g.r) }) g.emit("}\n\n") g.emit("// UnmarshalUnsafe implements marshal.Marshallable.UnmarshalUnsafe.\n") g.emit("func (%s *%s) UnmarshalUnsafe(src []byte) {\n", g.r, g.typeName()) g.inIndent(func() { - g.emit("safecopy.CopyOut(unsafe.Pointer(%s), src)\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(len(src)))\n", g.r) }) g.emit("}\n\n") @@ -260,11 +260,9 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emit("}\n") g.emit("size := (*%s)(nil).SizeBytes()\n\n", g.typeName()) - g.emitNoEscapeSliceDataPointer("&src", "val") - - g.emit("length, err := safecopy.CopyIn(dst[:(size*count)], val)\n") - g.emitKeepAlive("src") - g.emit("return length, err\n") + g.emit("dst = dst[:size*count]\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&src[0]), uintptr(len(dst)))\n") + g.emit("return size*count, nil\n") }) g.emit("}\n\n") @@ -279,11 +277,9 @@ func (g *interfaceGenerator) emitMarshallableSliceForPrimitiveNewtype(nt *ast.Id g.emit("}\n") g.emit("size := (*%s)(nil).SizeBytes()\n\n", g.typeName()) - g.emitNoEscapeSliceDataPointer("&dst", "val") - - g.emit("length, err := safecopy.CopyOut(val, src[:(size*count)])\n") - g.emitKeepAlive("dst") - g.emit("return length, err\n") + g.emit("src = src[:(size*count)]\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&src[0]), uintptr(len(src)))\n") + g.emit("return size*count, nil\n") }) g.emit("}\n\n") } diff --git a/tools/go_marshal/gomarshal/generator_interfaces_struct.go b/tools/go_marshal/gomarshal/generator_interfaces_struct.go index 72df1ab64..087c88b54 100644 --- a/tools/go_marshal/gomarshal/generator_interfaces_struct.go +++ b/tools/go_marshal/gomarshal/generator_interfaces_struct.go @@ -270,18 +270,18 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("%s.MarshalBytes(dst)\n", g.r) } if thisPacked { - g.recordUsedImport("safecopy") + g.recordUsedImport("gohacks") g.recordUsedImport("unsafe") if cond, ok := g.areFieldsPackedExpression(); ok { g.emit("if %s {\n", cond) g.inIndent(func() { - g.emit("safecopy.CopyIn(dst, unsafe.Pointer(%s))\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(%s), uintptr(len(dst)))\n", g.r) }) g.emit("} else {\n") g.inIndent(fallback) g.emit("}\n") } else { - g.emit("safecopy.CopyIn(dst, unsafe.Pointer(%s))\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(%s), uintptr(len(dst)))\n", g.r) } } else { fallback() @@ -297,25 +297,23 @@ func (g *interfaceGenerator) emitMarshallableForStruct(st *ast.StructType) { g.emit("%s.UnmarshalBytes(src)\n", g.r) } if thisPacked { - g.recordUsedImport("safecopy") - g.recordUsedImport("unsafe") + g.recordUsedImport("gohacks") if cond, ok := g.areFieldsPackedExpression(); ok { g.emit("if %s {\n", cond) g.inIndent(func() { - g.emit("safecopy.CopyOut(unsafe.Pointer(%s), src)\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(len(src)))\n", g.r) }) g.emit("} else {\n") g.inIndent(fallback) g.emit("}\n") } else { - g.emit("safecopy.CopyOut(unsafe.Pointer(%s), src)\n", g.r) + g.emit("gohacks.Memmove(unsafe.Pointer(%s), unsafe.Pointer(&src[0]), uintptr(len(src)))\n", g.r) } } else { fallback() } }) g.emit("}\n\n") - g.emit("// CopyOutN implements marshal.Marshallable.CopyOutN.\n") g.emit("//go:nosplit\n") g.recordUsedImport("marshal") @@ -561,16 +559,15 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, g.recordUsedImport("reflect") g.recordUsedImport("runtime") g.recordUsedImport("unsafe") + g.recordUsedImport("gohacks") if _, ok := g.areFieldsPackedExpression(); ok { g.emit("if !src[0].Packed() {\n") g.inIndent(fallback) g.emit("}\n\n") } - g.emitNoEscapeSliceDataPointer("&src", "val") - - g.emit("length, err := safecopy.CopyIn(dst[:(size*count)], val)\n") - g.emitKeepAlive("src") - g.emit("return length, err\n") + g.emit("dst = dst[:size*count]\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&src[0]), uintptr(len(dst)))\n") + g.emit("return size * count, nil\n") } else { fallback() } @@ -598,19 +595,19 @@ func (g *interfaceGenerator) emitMarshallableSliceForStruct(st *ast.StructType, g.emit("return size * count, nil\n") } if thisPacked { + g.recordUsedImport("gohacks") g.recordUsedImport("reflect") g.recordUsedImport("runtime") - g.recordUsedImport("unsafe") if _, ok := g.areFieldsPackedExpression(); ok { g.emit("if !dst[0].Packed() {\n") g.inIndent(fallback) g.emit("}\n\n") } - g.emitNoEscapeSliceDataPointer("&dst", "val") - g.emit("length, err := safecopy.CopyOut(val, src[:(size*count)])\n") - g.emitKeepAlive("dst") - g.emit("return length, err\n") + g.emit("src = src[:(size*count)]\n") + g.emit("gohacks.Memmove(unsafe.Pointer(&dst[0]), unsafe.Pointer(&src[0]), uintptr(len(src)))\n") + + g.emit("return count*size, nil\n") } else { fallback() } -- cgit v1.2.3