From a7fe9ada10c02a7ea61b2909ef7db151d290073f Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Wed, 8 Feb 2023 15:06:08 -0500 Subject: [PATCH] cmd/internal/cov: fix misuse of bufio.Reader.Read in read helper Fix a misuse of bufio.Reader.Read in the helper class cmd/internal/cov.MReader; the MReader method in question should have been using io.ReadFull (passing the bufio.Reader) instead of directly calling Read. Using the Read method instead of io.ReadFull will result in a "short" read when processing a specific subset of counter data files, e.g. those that are short enough to not trigger the mmap-based scheme we use for larger files, but also with a large args section (something large enough to exceed the default 4k buffer size used by bufio.Reader). Along the way, add some additional defered Close() calls for files opened by the CovDataReader.visitPod, to enure we don't leave any open file descriptor following a call to CovDataReader.Visit. Fixes #58411. Change-Id: Iea48dc25c0081be1ade29f3a633df02a681fd941 Reviewed-on: https://go-review.googlesource.com/c/go/+/466677 Run-TryBot: Than McIntosh TryBot-Result: Gopher Robot Reviewed-by: David Chase --- src/cmd/internal/cov/mreader.go | 2 +- src/cmd/internal/cov/read_test.go | 98 ++++++++++++++++++++++++++ src/cmd/internal/cov/readcovdata.go | 4 ++ src/cmd/internal/cov/testdata/small.go | 7 ++ 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 src/cmd/internal/cov/read_test.go create mode 100644 src/cmd/internal/cov/testdata/small.go diff --git a/src/cmd/internal/cov/mreader.go b/src/cmd/internal/cov/mreader.go index 17dcfff05ba0b..30f53d6ec1a47 100644 --- a/src/cmd/internal/cov/mreader.go +++ b/src/cmd/internal/cov/mreader.go @@ -51,7 +51,7 @@ func (r *MReader) Read(p []byte) (int, error) { r.off += int64(amt) return amt, nil } - return r.rdr.Read(p) + return io.ReadFull(r.rdr, p) } func (r *MReader) ReadByte() (byte, error) { diff --git a/src/cmd/internal/cov/read_test.go b/src/cmd/internal/cov/read_test.go new file mode 100644 index 0000000000000..cef03fa323e6e --- /dev/null +++ b/src/cmd/internal/cov/read_test.go @@ -0,0 +1,98 @@ +// Copyright 2022 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 cov_test + +import ( + "cmd/internal/cov" + "fmt" + "internal/coverage" + "internal/coverage/decodecounter" + "internal/coverage/decodemeta" + "internal/coverage/pods" + "internal/testenv" + "os" + "path/filepath" + "testing" +) + +// visitor implements the CovDataVisitor interface in a very stripped +// down way, just keeps track of interesting events. +type visitor struct { + metaFileCount int + counterFileCount int + funcCounterData int + metaFuncCount int +} + +func (v *visitor) BeginPod(p pods.Pod) {} +func (v *visitor) EndPod(p pods.Pod) {} +func (v *visitor) VisitMetaDataFile(mdf string, mfr *decodemeta.CoverageMetaFileReader) { + v.metaFileCount++ +} +func (v *visitor) BeginCounterDataFile(cdf string, cdr *decodecounter.CounterDataReader, dirIdx int) { + v.counterFileCount++ +} +func (v *visitor) EndCounterDataFile(cdf string, cdr *decodecounter.CounterDataReader, dirIdx int) {} +func (v *visitor) VisitFuncCounterData(payload decodecounter.FuncPayload) { v.funcCounterData++ } +func (v *visitor) EndCounters() {} +func (v *visitor) BeginPackage(pd *decodemeta.CoverageMetaDataDecoder, pkgIdx uint32) {} +func (v *visitor) EndPackage(pd *decodemeta.CoverageMetaDataDecoder, pkgIdx uint32) {} +func (v *visitor) VisitFunc(pkgIdx uint32, fnIdx uint32, fd *coverage.FuncDesc) { v.metaFuncCount++ } +func (v *visitor) Finish() {} + +func TestIssue58411(t *testing.T) { + testenv.MustHaveGoBuild(t) + + // Build a tiny test program with -cover. Smallness is important; + // it is one of the factors that triggers issue 58411. + d := t.TempDir() + exepath := filepath.Join(d, "small.exe") + path := filepath.Join("testdata", "small.go") + cmd := testenv.Command(t, testenv.GoToolPath(t), "build", + "-o", exepath, "-cover", path) + b, err := cmd.CombinedOutput() + if len(b) != 0 { + t.Logf("## build output:\n%s", b) + } + if err != nil { + t.Fatalf("build error: %v", err) + } + + // Run to produce coverage data. Note the large argument; we need a large + // argument (more than 4k) to trigger the bug, but the overall file + // has to remain small (since large files will be read with mmap). + covdir := filepath.Join(d, "covdata") + if err = os.Mkdir(covdir, 0777); err != nil { + t.Fatalf("creating covdir: %v", err) + } + large := fmt.Sprintf("%07999d", 0) + cmd = testenv.Command(t, exepath, "1", "2", "3", large) + cmd.Dir = covdir + cmd.Env = append(os.Environ(), "GOCOVERDIR="+covdir) + b, err = cmd.CombinedOutput() + if err != nil { + t.Logf("## run output:\n%s", b) + t.Fatalf("build error: %v", err) + } + + vis := &visitor{} + + // Read resulting coverage data. Without the fix, this would + // yield a "short read" error. + const verbosityLevel = 0 + const flags = 0 + cdr := cov.MakeCovDataReader(vis, []string{covdir}, verbosityLevel, flags, nil) + err = cdr.Visit() + if err != nil { + t.Fatalf("visit failed: %v", err) + } + + // make sure we saw a few things just for grins + const want = "{metaFileCount:1 counterFileCount:1 funcCounterData:1 metaFuncCount:1}" + got := fmt.Sprintf("%+v", *vis) + if want != got { + t.Errorf("visitor contents: want %v got %v\n", want, got) + } +} diff --git a/src/cmd/internal/cov/readcovdata.go b/src/cmd/internal/cov/readcovdata.go index 263148b993a84..7e90e9e808927 100644 --- a/src/cmd/internal/cov/readcovdata.go +++ b/src/cmd/internal/cov/readcovdata.go @@ -186,6 +186,7 @@ func (r *CovDataReader) visitPod(p pods.Pod) error { if err != nil { return r.fatal("unable to open meta-file %s", p.MetaFile) } + defer f.Close() br := bio.NewReader(f) fi, err := f.Stat() if err != nil { @@ -209,6 +210,9 @@ func (r *CovDataReader) visitPod(p pods.Pod) error { if err != nil { return r.fatal("opening counter data file %s: %s", cdf, err) } + defer func(f *os.File) { + f.Close() + }(cf) var mr *MReader mr, err = NewMreader(cf) if err != nil { diff --git a/src/cmd/internal/cov/testdata/small.go b/src/cmd/internal/cov/testdata/small.go new file mode 100644 index 0000000000000..d81cb70624c69 --- /dev/null +++ b/src/cmd/internal/cov/testdata/small.go @@ -0,0 +1,7 @@ +package main + +import "os" + +func main() { + println(len(os.Args)) +}