From 7038a380bcd0183842471c1984da491e044d3d34 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Wed, 3 Mar 2021 16:30:22 -0500 Subject: [release-branch.go1.15] cmd/go/internal/modfetch: detect and recover from missing ziphash file Previously, if an extracted module directory existed in the module cache, but the corresponding ziphash file did not, if the sum was missing from go.sum, we would not verify the sum. This caused 'go get' not to write missing sums. 'go build' in readonly mode (now the default) checks for missing sums and doesn't attempt to fetch modules that can't be verified against go.sum. With this change, when requesting the module directory with modfetch.DownloadDir, if the ziphash file is missing, the go command will re-hash the zip without downloading or re-extracting it again. Note that the go command creates the ziphash file before the module directory, but another program could remove it separately, and it might not be present after a crash. Fixes #44872 Change-Id: I64551e048a3ba17d069de1ec123d5b8b2757543c Reviewed-on: https://go-review.googlesource.com/c/go/+/298352 Trust: Jay Conrod Run-TryBot: Jay Conrod TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills (cherry picked from commit 302a400316319501748c0f034464fa70e7815272) Reviewed-on: https://go-review.googlesource.com/c/go/+/299830 --- src/cmd/go/internal/modfetch/cache.go | 17 +++++ src/cmd/go/internal/modfetch/fetch.go | 77 ++++++++++++++-------- .../go/testdata/script/mod_get_missing_ziphash.txt | 55 ++++++++++++++++ src/cmd/go/testdata/script/mod_verify.txt | 7 +- 4 files changed, 125 insertions(+), 31 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_get_missing_ziphash.txt diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go index e3074b775e..0b64a6943b 100644 --- a/src/cmd/go/internal/modfetch/cache.go +++ b/src/cmd/go/internal/modfetch/cache.go @@ -83,6 +83,7 @@ func DownloadDir(m module.Version) (string, error) { return "", err } + // Check whether the directory itself exists. dir := filepath.Join(cfg.GOMODCACHE, enc+"@"+encVer) if fi, err := os.Stat(dir); os.IsNotExist(err) { return dir, err @@ -91,6 +92,9 @@ func DownloadDir(m module.Version) (string, error) { } else if !fi.IsDir() { return dir, &DownloadDirPartialError{dir, errors.New("not a directory")} } + + // Check if a .partial file exists. This is created at the beginning of + // a download and removed after the zip is extracted. partialPath, err := CachePath(m, "partial") if err != nil { return dir, err @@ -100,6 +104,19 @@ func DownloadDir(m module.Version) (string, error) { } else if !os.IsNotExist(err) { return dir, err } + + // Check if a .ziphash file exists. It should be created before the + // zip is extracted, but if it was deleted (by another program?), we need + // to re-calculate it. + ziphashPath, err := CachePath(m, "ziphash") + if err != nil { + return dir, err + } + if _, err := os.Stat(ziphashPath); os.IsNotExist(err) { + return dir, &DownloadDirPartialError{dir, errors.New("ziphash file is missing")} + } else if err != nil { + return dir, err + } return dir, nil } diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index fd7a5cef83..6a6c972157 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -206,13 +206,16 @@ func DownloadZip(mod module.Version) (zipfile string, err error) { if err != nil { return cached{"", err} } + ziphashfile := zipfile + "hash" - // Skip locking if the zipfile already exists. + // Return without locking if the zip and ziphash files exist. if _, err := os.Stat(zipfile); err == nil { - return cached{zipfile, nil} + if _, err := os.Stat(ziphashfile); err == nil { + return cached{zipfile, nil} + } } - // The zip file does not exist. Acquire the lock and create it. + // The zip or ziphash file does not exist. Acquire the lock and create them. if cfg.CmdName != "mod download" { fmt.Fprintf(os.Stderr, "go: downloading %s %s\n", mod.Path, mod.Version) } @@ -222,14 +225,6 @@ func DownloadZip(mod module.Version) (zipfile string, err error) { } defer unlock() - // Double-check that the zipfile was not created while we were waiting for - // the lock. - if _, err := os.Stat(zipfile); err == nil { - return cached{zipfile, nil} - } - if err := os.MkdirAll(filepath.Dir(zipfile), 0777); err != nil { - return cached{"", err} - } if err := downloadZip(mod, zipfile); err != nil { return cached{"", err} } @@ -239,6 +234,25 @@ func DownloadZip(mod module.Version) (zipfile string, err error) { } func downloadZip(mod module.Version, zipfile string) (err error) { + // Double-check that the zipfile was not created while we were waiting for + // the lock in DownloadZip. + ziphashfile := zipfile + "hash" + var zipExists, ziphashExists bool + if _, err := os.Stat(zipfile); err == nil { + zipExists = true + } + if _, err := os.Stat(ziphashfile); err == nil { + ziphashExists = true + } + if zipExists && ziphashExists { + return nil + } + + // Create parent directories. + if err := os.MkdirAll(filepath.Dir(zipfile), 0777); err != nil { + return err + } + // Clean up any remaining tempfiles from previous runs. // This is only safe to do because the lock file ensures that their // writers are no longer active. @@ -250,6 +264,12 @@ func downloadZip(mod module.Version, zipfile string) (err error) { } } + // If the zip file exists, the ziphash file must have been deleted + // or lost after a file system crash. Re-hash the zip without downloading. + if zipExists { + return hashZip(mod, zipfile, ziphashfile) + } + // From here to the os.Rename call below is functionally almost equivalent to // renameio.WriteToFile, with one key difference: we want to validate the // contents of the file (by hashing it) before we commit it. Because the file @@ -306,15 +326,7 @@ func downloadZip(mod module.Version, zipfile string) (err error) { } // Hash the zip file and check the sum before renaming to the final location. - hash, err := dirhash.HashZip(f.Name(), dirhash.DefaultHash) - if err != nil { - return err - } - if err := checkModSum(mod, hash); err != nil { - return err - } - - if err := renameio.WriteFile(zipfile+"hash", []byte(hash), 0666); err != nil { + if err := hashZip(mod, f.Name(), ziphashfile); err != nil { return err } if err := os.Rename(f.Name(), zipfile); err != nil { @@ -326,6 +338,22 @@ func downloadZip(mod module.Version, zipfile string) (err error) { return nil } +// hashZip reads the zip file opened in f, then writes the hash to ziphashfile, +// overwriting that file if it exists. +// +// If the hash does not match go.sum (or the sumdb if enabled), hashZip returns +// an error and does not write ziphashfile. +func hashZip(mod module.Version, zipfile, ziphashfile string) error { + hash, err := dirhash.HashZip(zipfile, dirhash.DefaultHash) + if err != nil { + return err + } + if err := checkModSum(mod, hash); err != nil { + return err + } + return renameio.WriteFile(ziphashfile, []byte(hash), 0666) +} + // makeDirsReadOnly makes a best-effort attempt to remove write permissions for dir // and its transitive contents. func makeDirsReadOnly(dir string) { @@ -457,11 +485,6 @@ func readGoSum(dst map[module.Version][]string, file string, data []byte) error // checkMod checks the given module's checksum. func checkMod(mod module.Version) { - if cfg.GOMODCACHE == "" { - // Do not use current directory. - return - } - // Do the file I/O before acquiring the go.sum lock. ziphash, err := CachePath(mod, "ziphash") if err != nil { @@ -469,10 +492,6 @@ func checkMod(mod module.Version) { } data, err := renameio.ReadFile(ziphash) if err != nil { - if errors.Is(err, os.ErrNotExist) { - // This can happen if someone does rm -rf GOPATH/src/cache/download. So it goes. - return - } base.Fatalf("verifying %v", module.VersionError(mod, err)) } h := strings.TrimSpace(string(data)) diff --git a/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt b/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt new file mode 100644 index 0000000000..afb5d2e844 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt @@ -0,0 +1,55 @@ +# Test that if the module cache contains an extracted source directory but not +# a ziphash, 'go build' complains about a missing sum, and 'go get' adds +# the sum. Verifies #44749. + +# With a tidy go.sum, go build succeeds. This also populates the module cache. +cp go.sum.tidy go.sum +go build -n use +env GOPROXY=off +env GOSUMDB=off + +# Control case: if we delete the hash for rsc.io/quote v1.5.2, +# 'go build' reports an error. 'go get' adds the sum. +cp go.sum.bug go.sum +! go build -n -mod=readonly use +stderr '^go: updates to go.sum needed, disabled by -mod=readonly$' +go get -d use +cmp go.sum go.sum.tidy +go build -n use + +# If we delete the hash *and* the ziphash file, we should see the same behavior. +cp go.sum.bug go.sum +rm $WORK/gopath/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.ziphash +! go build -n -mod=readonly use +stderr '^go: updates to go.sum needed, disabled by -mod=readonly$' +go get -d use +cmp go.sum go.sum.tidy +go build -n use + +-- go.mod -- +module use + +go 1.17 + +require rsc.io/quote v1.5.2 +-- go.sum.tidy -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- go.sum.bug -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- use.go -- +package use + +import _ "rsc.io/quote" diff --git a/src/cmd/go/testdata/script/mod_verify.txt b/src/cmd/go/testdata/script/mod_verify.txt index 646bc62bb7..7163d1a64b 100644 --- a/src/cmd/go/testdata/script/mod_verify.txt +++ b/src/cmd/go/testdata/script/mod_verify.txt @@ -50,10 +50,13 @@ go mod tidy grep '^rsc.io/quote v1.1.0/go.mod ' go.sum grep '^rsc.io/quote v1.1.0 ' go.sum -# sync should ignore missing ziphash; verify should not +# verify should fail on a missing ziphash. tidy should restore it. rm $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.1.0.ziphash -go mod tidy ! go mod verify +stderr '^rsc.io/quote v1.1.0: missing ziphash: open '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]rsc.io[/\\]quote[/\\]@v[/\\]v1.1.0.ziphash' +go mod tidy +exists $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.1.0.ziphash +go mod verify # Packages below module root should not be mentioned in go.sum. rm go.sum -- cgit v1.2.3-54-g00ecf From 7c88ae4117c16c1e5b80189b81d67845e1c40d9d Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Tue, 16 Feb 2021 10:20:58 -0500 Subject: [release-branch.go1.15] cmd/link: generate trampoline for inter-dependent packages Currently, in the trampoline generation pass we expect packages are laid out in dependency order, so a cross-package jump always has a known target address so we can check if a trampoline is needed. With linknames, there can be cycles in the package dependency graph, making this algorithm no longer work. For them, as the target address is unkown we conservatively generate a trampoline. This may generate unnecessary trampolines (if the packages turn out laid together), but package cycles are extremely rare so this is fine. Updates #44639. Fixes #44748. Change-Id: I2dc2998edacbda27d726fc79452313a21d07787a Reviewed-on: https://go-review.googlesource.com/c/go/+/292490 Trust: Cherry Zhang Reviewed-by: Than McIntosh (cherry picked from commit 098504c73ff6ece19566a1ac811ceed73be7c81d) Reviewed-on: https://go-review.googlesource.com/c/go/+/298030 Run-TryBot: Cherry Zhang TryBot-Result: Go Bot --- src/cmd/link/internal/arm/asm.go | 16 +++++++++++----- src/cmd/link/internal/ld/data.go | 12 +++++------- src/cmd/link/internal/ppc64/asm.go | 12 +++++++++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/cmd/link/internal/arm/asm.go b/src/cmd/link/internal/arm/asm.go index 903e62108e..43643341d2 100644 --- a/src/cmd/link/internal/arm/asm.go +++ b/src/cmd/link/internal/arm/asm.go @@ -373,10 +373,16 @@ func trampoline(ctxt *ld.Link, ldr *loader.Loader, ri int, rs, s loader.Sym) { r := relocs.At2(ri) switch r.Type() { case objabi.R_CALLARM: - // r.Add is the instruction - // low 24-bit encodes the target address - t := (ldr.SymValue(rs) + int64(signext24(r.Add()&0xffffff)*4) - (ldr.SymValue(s) + int64(r.Off()))) / 4 - if t > 0x7fffff || t < -0x800000 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { + var t int64 + // ldr.SymValue(rs) == 0 indicates a cross-package jump to a function that is not yet + // laid out. Conservatively use a trampoline. This should be rare, as we lay out packages + // in dependency order. + if ldr.SymValue(rs) != 0 { + // r.Add is the instruction + // low 24-bit encodes the target address + t = (ldr.SymValue(rs) + int64(signext24(r.Add()&0xffffff)*4) - (ldr.SymValue(s) + int64(r.Off()))) / 4 + } + if t > 0x7fffff || t < -0x800000 || ldr.SymValue(rs) == 0 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { // direct call too far, need to insert trampoline. // look up existing trampolines first. if we found one within the range // of direct call, we can reuse it. otherwise create a new one. @@ -447,7 +453,7 @@ func gentramp(arch *sys.Arch, linkmode ld.LinkMode, ldr *loader.Loader, tramp *l arch.ByteOrder.PutUint32(P[8:], o3) tramp.SetData(P) - if linkmode == ld.LinkExternal { + if linkmode == ld.LinkExternal || ldr.SymValue(target) == 0 { r := loader.Reloc{ Off: 8, Type: objabi.R_ADDR, diff --git a/src/cmd/link/internal/ld/data.go b/src/cmd/link/internal/ld/data.go index d3f308c8fa..2b55a5f6fc 100644 --- a/src/cmd/link/internal/ld/data.go +++ b/src/cmd/link/internal/ld/data.go @@ -105,14 +105,12 @@ func trampoline(ctxt *Link, s loader.Sym) { } rs = ldr.ResolveABIAlias(rs) if ldr.SymValue(rs) == 0 && (ldr.SymType(rs) != sym.SDYNIMPORT && ldr.SymType(rs) != sym.SUNDEFEXT) { - if ldr.SymPkg(rs) != ldr.SymPkg(s) { - if !isRuntimeDepPkg(ldr.SymPkg(s)) || !isRuntimeDepPkg(ldr.SymPkg(rs)) { - ctxt.Errorf(s, "unresolved inter-package jump to %s(%s) from %s", ldr.SymName(rs), ldr.SymPkg(rs), ldr.SymPkg(s)) - } - // runtime and its dependent packages may call to each other. - // they are fine, as they will be laid down together. + if ldr.SymPkg(rs) == ldr.SymPkg(s) { + continue // symbols in the same package are laid out together + } + if isRuntimeDepPkg(ldr.SymPkg(s)) && isRuntimeDepPkg(ldr.SymPkg(rs)) { + continue // runtime packages are laid out together } - continue } thearch.Trampoline(ctxt, ldr, ri, rs, s) diff --git a/src/cmd/link/internal/ppc64/asm.go b/src/cmd/link/internal/ppc64/asm.go index b1c0873d7a..a3ecd43f89 100644 --- a/src/cmd/link/internal/ppc64/asm.go +++ b/src/cmd/link/internal/ppc64/asm.go @@ -672,13 +672,19 @@ func trampoline(ctxt *ld.Link, ldr *loader.Loader, ri int, rs, s loader.Sym) { relocs := ldr.Relocs(s) r := relocs.At2(ri) - t := ldr.SymValue(rs) + r.Add() - (ldr.SymValue(s) + int64(r.Off())) + var t int64 + // ldr.SymValue(rs) == 0 indicates a cross-package jump to a function that is not yet + // laid out. Conservatively use a trampoline. This should be rare, as we lay out packages + // in dependency order. + if ldr.SymValue(rs) != 0 { + t = ldr.SymValue(rs) + r.Add() - (ldr.SymValue(s) + int64(r.Off())) + } switch r.Type() { case objabi.R_CALLPOWER: // If branch offset is too far then create a trampoline. - if (ctxt.IsExternal() && ldr.SymSect(s) != ldr.SymSect(rs)) || (ctxt.IsInternal() && int64(int32(t<<6)>>6) != t) || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { + if (ctxt.IsExternal() && ldr.SymSect(s) != ldr.SymSect(rs)) || (ctxt.IsInternal() && int64(int32(t<<6)>>6) != t) || ldr.SymValue(rs) == 0 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { var tramp loader.Sym for i := 0; ; i++ { @@ -769,7 +775,7 @@ func gentramp(ctxt *ld.Link, ldr *loader.Loader, tramp *loader.SymbolBuilder, ta // With external linking, the target address must be // relocated using LO and HA - if ctxt.IsExternal() { + if ctxt.IsExternal() || ldr.SymValue(target) == 0 { r := loader.Reloc{ Off: 0, Type: objabi.R_ADDRPOWER, -- cgit v1.2.3-54-g00ecf From 69b943102868747ff2b62d9ada02c464d9984aef Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 29 Mar 2021 11:19:23 -0400 Subject: [release-branch.go1.15] net/http: update bundled x/net/http2 Bring in the change in CL 304309 with: go mod edit -replace=golang.org/x/net=golang.org/x/net@release-branch.go1.15-bundle GOFLAGS='-mod=mod' go generate -run=bundle std go mod edit -dropreplace=golang.org/x/net go get -d golang.org/x/net@release-branch.go1.15 go mod tidy go mod vendor For #45076. Updates #40213. Change-Id: I68d5e1f2394508c9cf8627fb852dd9e906d45016 Reviewed-on: https://go-review.googlesource.com/c/go/+/305489 Trust: Dmitri Shuralyov Trust: Emmanuel Odeke Run-TryBot: Dmitri Shuralyov Reviewed-by: Emmanuel Odeke TryBot-Result: Go Bot --- src/net/http/h2_bundle.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go index 678c6eb9d4..3d83084702 100644 --- a/src/net/http/h2_bundle.go +++ b/src/net/http/h2_bundle.go @@ -8985,6 +8985,8 @@ func http2strSliceContains(ss []string, s string) bool { type http2erringRoundTripper struct{ err error } +func (rt http2erringRoundTripper) RoundTripErr() error { return rt.err } + func (rt http2erringRoundTripper) RoundTrip(*Request) (*Response, error) { return nil, rt.err } // gzipReader wraps a response body so it can lazily -- cgit v1.2.3-54-g00ecf From be872812063f29e9c0d280fb37289d40330d96db Mon Sep 17 00:00:00 2001 From: Michael Fraenkel Date: Thu, 16 Jul 2020 21:30:12 -0600 Subject: [release-branch.go1.15] net/http: fix detection of Roundtrippers that always error CL 220905 added code to identify alternate transports that always error by using http2erringRoundTripper. This does not work when the transport is from another package, e.g., http2.erringRoundTripper. Expose a new method that allow detection of such a RoundTripper. Switch to an interface that is both a RoundTripper and can return the underlying error. Fixes #45076. Updates #40213. Change-Id: I170739857ab9e99dffb5fa55c99b24b23c2f9c54 Reviewed-on: https://go-review.googlesource.com/c/go/+/243258 Reviewed-by: Emmanuel Odeke Run-TryBot: Emmanuel Odeke TryBot-Result: Gobot Gobot Reviewed-on: https://go-review.googlesource.com/c/go/+/304210 Trust: Dmitri Shuralyov Trust: Emmanuel Odeke Run-TryBot: Dmitri Shuralyov TryBot-Result: Go Bot Reviewed-by: Emmanuel Odeke --- src/net/http/omithttp2.go | 4 ---- src/net/http/transport.go | 8 ++++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/net/http/omithttp2.go b/src/net/http/omithttp2.go index 7e2f492579..c8f5c28a59 100644 --- a/src/net/http/omithttp2.go +++ b/src/net/http/omithttp2.go @@ -32,10 +32,6 @@ type http2Transport struct { func (*http2Transport) RoundTrip(*Request) (*Response, error) { panic(noHTTP2) } func (*http2Transport) CloseIdleConnections() {} -type http2erringRoundTripper struct{ err error } - -func (http2erringRoundTripper) RoundTrip(*Request) (*Response, error) { panic(noHTTP2) } - type http2noDialH2RoundTripper struct{} func (http2noDialH2RoundTripper) RoundTrip(*Request) (*Response, error) { panic(noHTTP2) } diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 6e430b9885..88d15a5919 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -1531,6 +1531,10 @@ func (pconn *persistConn) addTLS(name string, trace *httptrace.ClientTrace) erro return nil } +type erringRoundTripper interface { + RoundTripErr() error +} + func (t *Transport) dialConn(ctx context.Context, cm connectMethod) (pconn *persistConn, err error) { pconn = &persistConn{ t: t, @@ -1697,9 +1701,9 @@ func (t *Transport) dialConn(ctx context.Context, cm connectMethod) (pconn *pers if s := pconn.tlsState; s != nil && s.NegotiatedProtocolIsMutual && s.NegotiatedProtocol != "" { if next, ok := t.TLSNextProto[s.NegotiatedProtocol]; ok { alt := next(cm.targetAddr, pconn.conn.(*tls.Conn)) - if e, ok := alt.(http2erringRoundTripper); ok { + if e, ok := alt.(erringRoundTripper); ok { // pconn.conn was closed by next (http2configureTransport.upgradeFn). - return nil, e.err + return nil, e.RoundTripErr() } return &persistConn{t: t, cacheKey: pconn.cacheKey, alt: alt}, nil } -- cgit v1.2.3-54-g00ecf From 6df5d3a581380a85183c591d1fd9063a5e8e4b23 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 26 Jan 2021 21:14:43 -0500 Subject: [release-branch.go1.15] build: set GOPATH consistently in run.bash, run.bat, run.rc We used to clear GOPATH in all the build scripts. Clearing GOPATH is misleading at best, since you just end up with the default GOPATH (%USERPROFILE%\go on Windows). Unless that's your GOROOT, in which case you end up with a fatal error from the go command (#43938). run.bash changed to setting GOPATH=/dev/null, which has no clear analogue on Windows. run.rc still clears GOPATH. Change them all to set GOPATH to a non-existent directory /nonexist-gopath or c:\nonexist-gopath. For #45238. Fixes #45239. Change-Id: I51edd66d37ff6a891b0d0541d91ecba97fbbb03d Reviewed-on: https://go-review.googlesource.com/c/go/+/288818 Trust: Russ Cox Trust: Jason A. Donenfeld Reviewed-by: Cherry Zhang Reviewed-by: Alex Brainman Reviewed-by: Jason A. Donenfeld (cherry picked from commit bb6efb96092cc8ae398c29e3b052a0051c746f88) Reviewed-on: https://go-review.googlesource.com/c/go/+/304773 Run-TryBot: Dmitri Shuralyov TryBot-Result: Go Bot Reviewed-by: Alexander Rakoczy Reviewed-by: Carlos Amedee Trust: Carlos Amedee --- src/run.bash | 10 +--------- src/run.bat | 4 +--- src/run.rc | 9 ++++----- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/run.bash b/src/run.bash index 706b4b60ee..2123c509f8 100755 --- a/src/run.bash +++ b/src/run.bash @@ -23,15 +23,7 @@ fi eval $(../bin/go env) export GOROOT # The api test requires GOROOT to be set, so set it to match ../bin/go. - -# We disallow local import for non-local packages, if $GOROOT happens -# to be under $GOPATH, then some tests below will fail. $GOPATH needs -# to be set to a non-empty string, else Go will set a default value -# that may also conflict with $GOROOT. The $GOPATH value doesn't need -# to point to an actual directory, it just needs to pass the semantic -# checks performed by Go. Use $GOROOT to define $GOPATH so that we -# don't blunder into a user-defined symbolic link. -export GOPATH=/dev/null +export GOPATH=/nonexist-gopath unset CDPATH # in case user has it set export GOBIN=$GOROOT/bin # Issue 14340 diff --git a/src/run.bat b/src/run.bat index c299671c13..edcaf52659 100644 --- a/src/run.bat +++ b/src/run.bat @@ -18,9 +18,7 @@ setlocal set GOBUILDFAIL=0 -:: we disallow local import for non-local packages, if %GOROOT% happens -:: to be under %GOPATH%, then some tests below will fail -set GOPATH= +set GOPATH=c:\nonexist-gopath :: Issue 14340: ignore GOBIN during all.bat. set GOBIN= set GOFLAGS= diff --git a/src/run.rc b/src/run.rc index ab7abfa991..a7b4801207 100755 --- a/src/run.rc +++ b/src/run.rc @@ -12,10 +12,9 @@ if(! test -f ../bin/go){ eval `{../bin/go env} -GOPATH = () # we disallow local import for non-local packages, if $GOROOT happens - # to be under $GOPATH, then some tests below will fail -GOBIN = () # Issue 14340 -GOFLAGS = () -GO111MODULE = () +GOPATH=/nonexist-gopath +GOBIN=() # Issue 14340 +GOFLAGS=() +GO111MODULE=() exec ../bin/go tool dist test -rebuild $* -- cgit v1.2.3-54-g00ecf From c77418f4cac41c42566ca90921a1f928995cfba2 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Mon, 18 Jan 2021 16:18:11 -0800 Subject: [release-branch.go1.15] database/sql: fix tx stmt deadlock when rollback Tx acquires tx.closemu W-lock and then acquires stmt.closemu.W-lock to fully close the transaction and associated prepared statement. Stmt query and execution run in reverse ways - acquires stmt.closemu.R-lock and then acquires tx.closemu.R-lock to grab tx connection, which may cause deadlock. Prevent the lock is held around tx.closePrepared to ensure no deadlock happens. Includes a test fix from CL 266097. Fixes #42884 Updates #40985 Updates #42259 Change-Id: Id52737660ada3cebdfff6efc23366cdc3224b8e8 Reviewed-on: https://go-review.googlesource.com/c/go/+/250178 Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot Reviewed-by: Daniel Theophanes Trust: Emmanuel Odeke (cherry picked from commit d4c1ad882973e407ff85b977f4ce5b9435451190) Reviewed-on: https://go-review.googlesource.com/c/go/+/284513 Reviewed-by: Dmitri Shuralyov --- src/database/sql/sql.go | 14 +++++++------- src/database/sql/sql_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index b3d0653f5c..3d1367d28f 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -2080,10 +2080,10 @@ func (tx *Tx) isDone() bool { // that has already been committed or rolled back. var ErrTxDone = errors.New("sql: transaction has already been committed or rolled back") -// closeLocked returns the connection to the pool and +// close returns the connection to the pool and // must only be called by Tx.rollback or Tx.Commit while -// closemu is Locked and tx already canceled. -func (tx *Tx) closeLocked(err error) { +// tx is already canceled and won't be executed concurrently. +func (tx *Tx) close(err error) { tx.releaseConn(err) tx.dc = nil tx.txi = nil @@ -2157,7 +2157,7 @@ func (tx *Tx) Commit() error { // to ensure no other connection has an active query. tx.cancel() tx.closemu.Lock() - defer tx.closemu.Unlock() + tx.closemu.Unlock() var err error withLock(tx.dc, func() { @@ -2166,7 +2166,7 @@ func (tx *Tx) Commit() error { if err != driver.ErrBadConn { tx.closePrepared() } - tx.closeLocked(err) + tx.close(err) return err } @@ -2189,7 +2189,7 @@ func (tx *Tx) rollback(discardConn bool) error { // to ensure no other connection has an active query. tx.cancel() tx.closemu.Lock() - defer tx.closemu.Unlock() + tx.closemu.Unlock() var err error withLock(tx.dc, func() { @@ -2201,7 +2201,7 @@ func (tx *Tx) rollback(discardConn bool) error { if discardConn { err = driver.ErrBadConn } - tx.closeLocked(err) + tx.close(err) return err } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 5727f0d8aa..d7d4642608 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2810,6 +2810,34 @@ func TestTxCannotCommitAfterRollback(t *testing.T) { } } +// Issue 40985 transaction statement deadlock while context cancel. +func TestTxStmtDeadlock(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) + defer cancel() + tx, err := db.BeginTx(ctx, nil) + if err != nil { + t.Fatal(err) + } + + stmt, err := tx.Prepare("SELECT|people|name,age|age=?") + if err != nil { + t.Fatal(err) + } + // Run number of stmt queries to reproduce deadlock from context cancel + for i := 0; i < 1e3; i++ { + // Encounter any close related errors (e.g.. ErrTxDone, stmt is closed) + // is expected due to context cancel. + _, err = stmt.Query(1) + if err != nil { + break + } + } + _ = tx.Rollback() +} + // Issue32530 encounters an issue where a connection may // expire right after it comes out of a used connection pool // even when a new connection is requested. -- cgit v1.2.3-54-g00ecf From f17b659d0a9a43235f67c155b08023124b3fc35b Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Tue, 23 Mar 2021 14:48:47 -0700 Subject: [release-branch.go1.15] cmd/compile: disable shortcircuit optimization for intertwined phi values We need to be careful that when doing value graph surgery, we not re-substitute a value that has already been substituted. That can lead to confusing a previous iteration's value with the current iteration's value. The simple fix in this CL just aborts the optimization if it detects intertwined phis (a phi which is the argument to another phi). It might be possible to keep the optimization with a more complicated CL, but: 1) This CL is clearly safe to backport. 2) There were no instances of this abort triggering in all.bash, prior to the test introduced in this CL. Fixes #45187 Change-Id: I2411dca03948653c053291f6829a76bec0c32330 Reviewed-on: https://go-review.googlesource.com/c/go/+/304251 Trust: Keith Randall Trust: Josh Bleecher Snyder Run-TryBot: Keith Randall TryBot-Result: Go Bot Reviewed-by: Josh Bleecher Snyder (cherry picked from commit 771c57e68ed5ef2bbb0eafc0d48419f59d143932) Reviewed-on: https://go-review.googlesource.com/c/go/+/304529 --- src/cmd/compile/internal/ssa/shortcircuit.go | 18 +++++++++++++++++ test/fixedbugs/issue45175.go | 29 ++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test/fixedbugs/issue45175.go diff --git a/src/cmd/compile/internal/ssa/shortcircuit.go b/src/cmd/compile/internal/ssa/shortcircuit.go index c5df457c4e..9cf29fe413 100644 --- a/src/cmd/compile/internal/ssa/shortcircuit.go +++ b/src/cmd/compile/internal/ssa/shortcircuit.go @@ -138,6 +138,24 @@ func shortcircuitBlock(b *Block) bool { if len(b.Values) != nval+nOtherPhi { return false } + if nOtherPhi > 0 { + // Check for any phi which is the argument of another phi. + // These cases are tricky, as substitutions done by replaceUses + // are no longer trivial to do in any ordering. See issue 45175. + m := make(map[*Value]bool, 1+nOtherPhi) + for _, v := range b.Values { + if v.Op == OpPhi { + m[v] = true + } + } + for v := range m { + for _, a := range v.Args { + if a != v && m[a] { + return false + } + } + } + } // Locate index of first const phi arg. cidx := -1 diff --git a/test/fixedbugs/issue45175.go b/test/fixedbugs/issue45175.go new file mode 100644 index 0000000000..02dfe8a0a9 --- /dev/null +++ b/test/fixedbugs/issue45175.go @@ -0,0 +1,29 @@ +// run + +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +//go:noinline +func f(c bool) int { + b := true + x := 0 + y := 1 + for b { + b = false + y = x + x = 2 + if c { + return 3 + } + } + return y +} + +func main() { + if got := f(false); got != 0 { + panic(got) + } +} -- cgit v1.2.3-54-g00ecf From a07f9d2f82598d2f5cc0df0e813a6a2309cb96e6 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Thu, 11 Mar 2021 12:28:45 -0500 Subject: [release-branch.go1.15] runtime: non-strict InlTreeIndex lookup in Frames.Next When using cgo, some of the frames can be provided by cgoTraceback, a cgo-provided function to generate C tracebacks. Unlike Go tracebacks, cgoTraceback has no particular guarantees that it produces valid tracebacks. If one of the (invalid) frames happens to put the PC in the alignment region at the end of a function (filled with int 3's on amd64), then Frames.Next will find a valid funcInfo for the PC, but pcdatavalue will panic because PCDATA doesn't cover this PC. Tolerate this case by doing a non-strict PCDATA lookup. We'll still show a bogus frame, but at least avoid throwing. For #44971 Fixes #45302 Change-Id: I9eed728470d6f264179a7615bd19845c941db78c Reviewed-on: https://go-review.googlesource.com/c/go/+/301369 Trust: Michael Pratt Run-TryBot: Michael Pratt TryBot-Result: Go Bot Reviewed-by: Cherry Zhang (cherry picked from commit e4a4161f1f3157550846e1b6bd4fe83aae15778e) Reviewed-on: https://go-review.googlesource.com/c/go/+/305890 --- src/runtime/symtab.go | 4 ++- src/runtime/symtab_test.go | 85 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index 1e86662adc..c77d513e74 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -102,7 +102,9 @@ func (ci *Frames) Next() (frame Frame, more bool) { name := funcname(funcInfo) if inldata := funcdata(funcInfo, _FUNCDATA_InlTree); inldata != nil { inltree := (*[1 << 20]inlinedCall)(inldata) - ix := pcdatavalue(funcInfo, _PCDATA_InlTreeIndex, pc, nil) + // Non-strict as cgoTraceback may have added bogus PCs + // with a valid funcInfo but invalid PCDATA. + ix := pcdatavalue1(funcInfo, _PCDATA_InlTreeIndex, pc, nil, false) if ix >= 0 { // Note: entry is not modified. It always refers to a real frame, not an inlined one. f = nil diff --git a/src/runtime/symtab_test.go b/src/runtime/symtab_test.go index 01e5002659..ffa07c7f3a 100644 --- a/src/runtime/symtab_test.go +++ b/src/runtime/symtab_test.go @@ -8,6 +8,7 @@ import ( "runtime" "strings" "testing" + "unsafe" ) func TestCaller(t *testing.T) { @@ -165,3 +166,87 @@ func TestNilName(t *testing.T) { t.Errorf("Name() = %q, want %q", got, "") } } + +var dummy int + +func inlined() { + // Side effect to prevent elimination of this entire function. + dummy = 42 +} + +// A function with an InlTree. Returns a PC within the function body. +// +// No inline to ensure this complete function appears in output. +// +//go:noinline +func tracebackFunc(t *testing.T) uintptr { + // This body must be more complex than a single call to inlined to get + // an inline tree. + inlined() + inlined() + + // Acquire a PC in this function. + pc, _, _, ok := runtime.Caller(0) + if !ok { + t.Fatalf("Caller(0) got ok false, want true") + } + + return pc +} + +// Test that CallersFrames handles PCs in the alignment region between +// functions (int 3 on amd64) without crashing. +// +// Go will never generate a stack trace containing such an address, as it is +// not a valid call site. However, the cgo traceback function passed to +// runtime.SetCgoTraceback may not be completely accurate and may incorrect +// provide PCs in Go code or the alignement region between functions. +// +// Go obviously doesn't easily expose the problematic PCs to running programs, +// so this test is a bit fragile. Some details: +// +// * tracebackFunc is our target function. We want to get a PC in the +// alignment region following this function. This function also has other +// functions inlined into it to ensure it has an InlTree (this was the source +// of the bug in issue 44971). +// +// * We acquire a PC in tracebackFunc, walking forwards until FuncForPC says +// we're in a new function. The last PC of the function according to FuncForPC +// should be in the alignment region (assuming the function isn't already +// perfectly aligned). +// +// This is a regression test for issue 44971. +func TestFunctionAlignmentTraceback(t *testing.T) { + pc := tracebackFunc(t) + + // Double-check we got the right PC. + f := runtime.FuncForPC(pc) + if !strings.HasSuffix(f.Name(), "tracebackFunc") { + t.Fatalf("Caller(0) = %+v, want tracebackFunc", f) + } + + // Iterate forward until we find a different function. Back up one + // instruction is (hopefully) an alignment instruction. + for runtime.FuncForPC(pc) == f { + pc++ + } + pc-- + + // Is this an alignment region filler instruction? We only check this + // on amd64 for simplicity. If this function has no filler, then we may + // get a false negative, but will never get a false positive. + if runtime.GOARCH == "amd64" { + code := *(*uint8)(unsafe.Pointer(pc)) + if code != 0xcc { // INT $3 + t.Errorf("PC %v code got %#x want 0xcc", pc, code) + } + } + + // Finally ensure that Frames.Next doesn't crash when processing this + // PC. + frames := runtime.CallersFrames([]uintptr{pc}) + frame, _ := frames.Next() + if frame.Func != f { + t.Errorf("frames.Next() got %+v want %+v", frame.Func, f) + } +} -- cgit v1.2.3-54-g00ecf From 5055314a5749664ab66a24beed8158552e276959 Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 15 Oct 2020 23:12:49 +0200 Subject: [release-branch.go1.15] cmd/cgo: avoid exporting all symbols on windows buildmode=c-shared Disable default symbol auto-export behaviour by marking exported function with the __declspec(dllexport) attribute. Old behaviour can still be used by setting -extldflags=-Wl,--export-all-symbols. See https://sourceware.org/binutils/docs/ld/WIN32.html for more info. This change cuts 50kb of a "hello world" dll. Updates #6853. Updates #30674. Fixes #43591. Change-Id: I9c7fb09c677cc760f24d0f7d199740ae73981413 Reviewed-on: https://go-review.googlesource.com/c/go/+/262797 Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor Reviewed-by: Alex Brainman Trust: Alex Brainman Reviewed-on: https://go-review.googlesource.com/c/go/+/300693 Trust: Dmitri Shuralyov Run-TryBot: Dmitri Shuralyov Reviewed-by: David Chase --- misc/cgo/testcshared/cshared_test.go | 96 ++++++++++++++++++++++++++++++++++++ src/cmd/cgo/out.go | 6 ++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/misc/cgo/testcshared/cshared_test.go b/misc/cgo/testcshared/cshared_test.go index bd4d341820..d717b4dfb3 100644 --- a/misc/cgo/testcshared/cshared_test.go +++ b/misc/cgo/testcshared/cshared_test.go @@ -7,6 +7,8 @@ package cshared_test import ( "bytes" "debug/elf" + "debug/pe" + "encoding/binary" "flag" "fmt" "io/ioutil" @@ -355,6 +357,100 @@ func TestExportedSymbols(t *testing.T) { } } +func checkNumberOfExportedFunctionsWindows(t *testing.T, exportAllSymbols bool) { + const prog = ` +package main + +import "C" + +//export GoFunc +func GoFunc() { + println(42) +} + +//export GoFunc2 +func GoFunc2() { + println(24) +} + +func main() { +} +` + + tmpdir := t.TempDir() + + srcfile := filepath.Join(tmpdir, "test.go") + objfile := filepath.Join(tmpdir, "test.dll") + if err := ioutil.WriteFile(srcfile, []byte(prog), 0666); err != nil { + t.Fatal(err) + } + argv := []string{"build", "-buildmode=c-shared"} + if exportAllSymbols { + argv = append(argv, "-ldflags", "-extldflags=-Wl,--export-all-symbols") + } + argv = append(argv, "-o", objfile, srcfile) + out, err := exec.Command("go", argv...).CombinedOutput() + if err != nil { + t.Fatalf("build failure: %s\n%s\n", err, string(out)) + } + + f, err := pe.Open(objfile) + if err != nil { + t.Fatalf("pe.Open failed: %v", err) + } + defer f.Close() + section := f.Section(".edata") + if section == nil { + t.Error(".edata section is not present") + } + + // TODO: deduplicate this struct from cmd/link/internal/ld/pe.go + type IMAGE_EXPORT_DIRECTORY struct { + _ [2]uint32 + _ [2]uint16 + _ [2]uint32 + NumberOfFunctions uint32 + NumberOfNames uint32 + _ [3]uint32 + } + var e IMAGE_EXPORT_DIRECTORY + if err := binary.Read(section.Open(), binary.LittleEndian, &e); err != nil { + t.Fatalf("binary.Read failed: %v", err) + } + + expectedNumber := uint32(2) + + if exportAllSymbols { + if e.NumberOfFunctions <= expectedNumber { + t.Fatalf("missing exported functions: %v", e.NumberOfFunctions) + } + if e.NumberOfNames <= expectedNumber { + t.Fatalf("missing exported names: %v", e.NumberOfNames) + } + } else { + if e.NumberOfFunctions != expectedNumber { + t.Fatalf("too many exported functions: %v", e.NumberOfFunctions) + } + if e.NumberOfNames != expectedNumber { + t.Fatalf("too many exported names: %v", e.NumberOfNames) + } + } +} + +func TestNumberOfExportedFunctions(t *testing.T) { + if GOOS != "windows" { + t.Skip("skipping windows only test") + } + t.Parallel() + + t.Run("OnlyExported", func(t *testing.T) { + checkNumberOfExportedFunctionsWindows(t, false) + }) + t.Run("All", func(t *testing.T) { + checkNumberOfExportedFunctionsWindows(t, true) + }) +} + // test1: shared library can be dynamically loaded and exported symbols are accessible. func TestExportedSymbolsWithDynamicLoad(t *testing.T) { t.Parallel() diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index b9043efbf7..ee1c563495 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -961,7 +961,11 @@ func (p *Package) writeExports(fgo2, fm, fgcc, fgcch io.Writer) { } // Build the wrapper function compiled by gcc. - s := fmt.Sprintf("%s %s(", gccResult, exp.ExpName) + gccExport := "" + if goos == "windows" { + gccExport = "__declspec(dllexport)" + } + s := fmt.Sprintf("%s %s %s(", gccExport, gccResult, exp.ExpName) if fn.Recv != nil { s += p.cgoType(fn.Recv.List[0].Type).C.String() s += " recv" -- cgit v1.2.3-54-g00ecf From 82f9c6cac1bcc6bc80f17d1415ac3f62c0afa6c5 Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 22 Oct 2020 22:32:20 +0200 Subject: [release-branch.go1.15] cmd/link: avoid exporting all symbols on windows buildmode=pie Marking one functions with __declspec(dllexport) forces mingw to create .reloc section without having to export all symbols. See https://insights.sei.cmu.edu/cert/2018/08/when-aslr-is-not-really-aslr---the-case-of-incorrect-assumptions-and-bad-defaults.html for more info. This change cuts 73kb of a "hello world" pie binary. Updates #6853. Updates #40795. Fixes #43592. Change-Id: I3cc57c3b64f61187550bc8751dfa085f106c8475 Reviewed-on: https://go-review.googlesource.com/c/go/+/264459 Trust: Alex Brainman Run-TryBot: Alex Brainman TryBot-Result: Go Bot Reviewed-by: Alex Brainman Reviewed-by: Austin Clements Reviewed-on: https://go-review.googlesource.com/c/go/+/300692 Trust: Dmitri Shuralyov Run-TryBot: Dmitri Shuralyov Reviewed-by: David Chase --- misc/cgo/testcshared/cshared_test.go | 9 +++++---- src/cmd/go/go_test.go | 33 +++++++++++++++++++++++++++++++++ src/cmd/link/internal/ld/lib.go | 3 --- src/runtime/cgo/gcc_windows_386.c | 1 + src/runtime/cgo/gcc_windows_amd64.c | 1 + src/runtime/cgo/libcgo_windows.h | 12 ++++++++++++ 6 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 src/runtime/cgo/libcgo_windows.h diff --git a/misc/cgo/testcshared/cshared_test.go b/misc/cgo/testcshared/cshared_test.go index d717b4dfb3..dff2682e20 100644 --- a/misc/cgo/testcshared/cshared_test.go +++ b/misc/cgo/testcshared/cshared_test.go @@ -401,7 +401,7 @@ func main() { defer f.Close() section := f.Section(".edata") if section == nil { - t.Error(".edata section is not present") + t.Fatalf(".edata section is not present") } // TODO: deduplicate this struct from cmd/link/internal/ld/pe.go @@ -418,7 +418,8 @@ func main() { t.Fatalf("binary.Read failed: %v", err) } - expectedNumber := uint32(2) + // Only the two exported functions and _cgo_dummy_export should be exported + expectedNumber := uint32(3) if exportAllSymbols { if e.NumberOfFunctions <= expectedNumber { @@ -429,10 +430,10 @@ func main() { } } else { if e.NumberOfFunctions != expectedNumber { - t.Fatalf("too many exported functions: %v", e.NumberOfFunctions) + t.Fatalf("got %d exported functions; want %d", e.NumberOfFunctions, expectedNumber) } if e.NumberOfNames != expectedNumber { - t.Fatalf("too many exported names: %v", e.NumberOfNames) + t.Fatalf("got %d exported names; want %d", e.NumberOfNames, expectedNumber) } } } diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 021930a8a8..3f790cdeab 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -9,6 +9,7 @@ import ( "debug/elf" "debug/macho" "debug/pe" + "encoding/binary" "flag" "fmt" "go/format" @@ -2166,6 +2167,38 @@ func testBuildmodePIE(t *testing.T, useCgo, setBuildmodeToPIE bool) { if (dc & pe.IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE) == 0 { t.Error("IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE flag is not set") } + if useCgo { + // Test that only one symbol is exported (#40795). + // PIE binaries don´t require .edata section but unfortunately + // binutils doesn´t generate a .reloc section unless there is + // at least one symbol exported. + // See https://sourceware.org/bugzilla/show_bug.cgi?id=19011 + section := f.Section(".edata") + if section == nil { + t.Fatalf(".edata section is not present") + } + // TODO: deduplicate this struct from cmd/link/internal/ld/pe.go + type IMAGE_EXPORT_DIRECTORY struct { + _ [2]uint32 + _ [2]uint16 + _ [2]uint32 + NumberOfFunctions uint32 + NumberOfNames uint32 + _ [3]uint32 + } + var e IMAGE_EXPORT_DIRECTORY + if err := binary.Read(section.Open(), binary.LittleEndian, &e); err != nil { + t.Fatalf("binary.Read failed: %v", err) + } + + // Only _cgo_dummy_export should be exported + if e.NumberOfFunctions != 1 { + t.Fatalf("got %d exported functions; want 1", e.NumberOfFunctions) + } + if e.NumberOfNames != 1 { + t.Fatalf("got %d exported names; want 1", e.NumberOfNames) + } + } default: panic("unreachable") } diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go index fc89759997..d03fb6cf91 100644 --- a/src/cmd/link/internal/ld/lib.go +++ b/src/cmd/link/internal/ld/lib.go @@ -1423,9 +1423,6 @@ func (ctxt *Link) hostlink() { if ctxt.Arch.PtrSize >= 8 { argv = append(argv, "-Wl,--high-entropy-va") } - // Work around binutils limitation that strips relocation table for dynamicbase. - // See https://sourceware.org/bugzilla/show_bug.cgi?id=19011 - argv = append(argv, "-Wl,--export-all-symbols") default: // ELF. if ctxt.UseRelro() { diff --git a/src/runtime/cgo/gcc_windows_386.c b/src/runtime/cgo/gcc_windows_386.c index 9184b91393..60cb011bf2 100644 --- a/src/runtime/cgo/gcc_windows_386.c +++ b/src/runtime/cgo/gcc_windows_386.c @@ -9,6 +9,7 @@ #include #include #include "libcgo.h" +#include "libcgo_windows.h" static void threadentry(void*); diff --git a/src/runtime/cgo/gcc_windows_amd64.c b/src/runtime/cgo/gcc_windows_amd64.c index 7192a24631..0f8c817f0e 100644 --- a/src/runtime/cgo/gcc_windows_amd64.c +++ b/src/runtime/cgo/gcc_windows_amd64.c @@ -9,6 +9,7 @@ #include #include #include "libcgo.h" +#include "libcgo_windows.h" static void threadentry(void*); diff --git a/src/runtime/cgo/libcgo_windows.h b/src/runtime/cgo/libcgo_windows.h new file mode 100644 index 0000000000..0013f06bae --- /dev/null +++ b/src/runtime/cgo/libcgo_windows.h @@ -0,0 +1,12 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Ensure there's one symbol marked __declspec(dllexport). +// If there are no exported symbols, the unfortunate behavior of +// the binutils linker is to also strip the relocations table, +// resulting in non-PIE binary. The other option is the +// --export-all-symbols flag, but we don't need to export all symbols +// and this may overflow the export table (#40795). +// See https://sourceware.org/bugzilla/show_bug.cgi?id=19011 +__declspec(dllexport) int _cgo_dummy_export; -- cgit v1.2.3-54-g00ecf From 1373d92a75f70c62d74f9e4714cd17811f75c48b Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 14 Jan 2021 21:29:49 +0100 Subject: [release-branch.go1.15] cmd/cgo: remove unnecessary space in cgo export header The cgo header has an unnecessary space in the exported function definition on non-windows goos. This was introduced in go1.16 so it would be good to fix it before release. Example: // Current behavior, notice there is an unecessary space // between extern and void extern void Foo(); // With this CL extern void Foo(); Updates #43591. Change-Id: Ic2c21f8d806fe35a7be7183dbfe35ac605b6e4f6 Reviewed-on: https://go-review.googlesource.com/c/go/+/283892 Reviewed-by: Ian Lance Taylor Trust: Katie Hockman Reviewed-on: https://go-review.googlesource.com/c/go/+/300694 Trust: Dmitri Shuralyov Trust: Emmanuel Odeke Run-TryBot: Dmitri Shuralyov TryBot-Result: Go Bot Reviewed-by: David Chase --- src/cmd/cgo/out.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index ee1c563495..be4f6ad2d5 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -963,9 +963,9 @@ func (p *Package) writeExports(fgo2, fm, fgcc, fgcch io.Writer) { // Build the wrapper function compiled by gcc. gccExport := "" if goos == "windows" { - gccExport = "__declspec(dllexport)" + gccExport = "__declspec(dllexport) " } - s := fmt.Sprintf("%s %s %s(", gccExport, gccResult, exp.ExpName) + s := fmt.Sprintf("%s%s %s(", gccExport, gccResult, exp.ExpName) if fn.Recv != nil { s += p.cgoType(fn.Recv.List[0].Type).C.String() s += " recv" -- cgit v1.2.3-54-g00ecf From 8c163e85267d146274f68854fe02b4a495586584 Mon Sep 17 00:00:00 2001 From: Carlos Amedee Date: Thu, 1 Apr 2021 10:09:59 -0400 Subject: [release-branch.go1.15] go1.15.11 Change-Id: I601bb0b69e8204055ce37150b50779818a339169 Reviewed-on: https://go-review.googlesource.com/c/go/+/306570 Run-TryBot: Carlos Amedee TryBot-Result: Go Bot Reviewed-by: David Chase Reviewed-by: Dmitri Shuralyov --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index b22abb18df..c25f9907d4 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.15.10 \ No newline at end of file +go1.15.11 \ No newline at end of file -- cgit v1.2.3-54-g00ecf