[release-branch.go1.20] 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 #58427.
Updates #58411.

Change-Id: Iea48dc25c0081be1ade29f3a633df02a681fd941
Reviewed-on: https://go-review.googlesource.com/c/go/+/466677
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit a7fe9ada10c02a7ea61b2909ef7db151d290073f)
Reviewed-on: https://go-review.googlesource.com/c/go/+/468536
Reviewed-by: Cherry Mui <cherryyz@google.com>

4 files changed, 110 insertions, 1 deletions

diff --git a/src/cmd/internal/cov/mreader.go b/src/cmd/internal/cov/mreader.go
index 17dcfff05b..30f53d6ec1 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 0000000000..cef03fa323
--- /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 263148b993..7e90e9e808 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 0000000000..d81cb70624
--- /dev/null
+++ b/src/cmd/internal/cov/testdata/small.go
@@ -0,0 +1,7 @@
+package main
+
+import "os"
+
+func main() {
+	println(len(os.Args))
+}