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 golang#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>

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) {

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)
+	}
+}

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 {

src/cmd/internal/cov/testdata/small.go

@@ -0,0 +1,7 @@
+package main
+
+import "os"
+
+func main() {
+	println(len(os.Args))
+}