diff --git a/pkg/yqlib/all_at_once_evaluator.go b/pkg/yqlib/all_at_once_evaluator.go index bdeddd20..cfc0624b 100644 --- a/pkg/yqlib/all_at_once_evaluator.go +++ b/pkg/yqlib/all_at_once_evaluator.go @@ -49,12 +49,13 @@ func (e *allAtOnceEvaluator) EvaluateFiles(expression string, filenames []string var allDocuments = list.New() for _, filename := range filenames { - reader, err := readStream(filename) + reader, cleanup, err := readStream(filename) if err != nil { return err } fileDocuments, err := readDocuments(reader, filename, fileIndex, decoder) + cleanup() if err != nil { return err } diff --git a/pkg/yqlib/front_matter_test.go b/pkg/yqlib/front_matter_test.go index 2680aa28..7de929ae 100644 --- a/pkg/yqlib/front_matter_test.go +++ b/pkg/yqlib/front_matter_test.go @@ -136,10 +136,11 @@ Some content test.AssertResult(t, originalFilename, resolved) // Read documents using the temp file, verify they get the original filename - reader, err := readStream(tempFilename) + reader, cleanup, err := readStream(tempFilename) if err != nil { panic(err) } + defer cleanup() decoder := NewYamlDecoder(ConfiguredYamlPreferences) docs, err := readDocuments(reader, tempFilename, 0, decoder) if err != nil { diff --git a/pkg/yqlib/stream_evaluator.go b/pkg/yqlib/stream_evaluator.go index ad147894..657c0401 100644 --- a/pkg/yqlib/stream_evaluator.go +++ b/pkg/yqlib/stream_evaluator.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "os" ) // A yaml expression evaluator that runs the expression multiple times for each given yaml document. @@ -50,21 +49,17 @@ func (s *streamEvaluator) EvaluateFiles(expression string, filenames []string, p } for _, filename := range filenames { - reader, err := readStream(filename) + reader, cleanup, err := readStream(filename) if err != nil { return err } processedDocs, err := s.Evaluate(filename, reader, node, printer, decoder) + cleanup() if err != nil { return err } totalProcessDocs = totalProcessDocs + processedDocs - - switch reader := reader.(type) { - case *os.File: - safelyCloseFile(reader) - } } if totalProcessDocs == 0 { diff --git a/pkg/yqlib/utils.go b/pkg/yqlib/utils.go index e4036f03..d7eb6928 100644 --- a/pkg/yqlib/utils.go +++ b/pkg/yqlib/utils.go @@ -32,21 +32,20 @@ func resolveFilename(filename string) string { return filename } -func readStream(filename string) (io.Reader, error) { - var reader *bufio.Reader +// readStream returns a reader for the given file, along with a cleanup function +// that must be called once the reader is no longer needed. The cleanup is a no-op +// for stdin. +func readStream(filename string) (io.Reader, func(), error) { if filename == "-" { - reader = bufio.NewReader(os.Stdin) - } else { - // ignore CWE-22 gosec issue - that's more targeted for http based apps that run in a public directory, - // and ensuring that it's not possible to give a path to a file outside that directory. - file, err := os.Open(filename) // #nosec - if err != nil { - return nil, err - } - reader = bufio.NewReader(file) + return bufio.NewReader(os.Stdin), func() {}, nil } - return reader, nil - + // ignore CWE-22 gosec issue - that's more targeted for http based apps that run in a public directory, + // and ensuring that it's not possible to give a path to a file outside that directory. + file, err := os.Open(filename) // #nosec + if err != nil { + return nil, nil, err + } + return bufio.NewReader(file), func() { safelyCloseFile(file) }, nil } func writeString(writer io.Writer, txt string) error { diff --git a/pkg/yqlib/utils_test.go b/pkg/yqlib/utils_test.go index fc1b0026..19d50069 100644 --- a/pkg/yqlib/utils_test.go +++ b/pkg/yqlib/utils_test.go @@ -4,11 +4,89 @@ import ( "bufio" "bytes" "io" + "os" + "path/filepath" + "runtime/debug" + "strconv" "testing" "github.com/mikefarah/yq/v4/test" ) +// countOpenFileDescriptors returns the number of file descriptors this process +// currently holds open, or -1 if the platform does not expose them. +func countOpenFileDescriptors() int { + for _, dir := range []string{"/proc/self/fd", "/dev/fd"} { + // Readdirnames avoids stat-ing each entry, which races with descriptors + // (including this directory handle) being closed underneath us. + handle, err := os.Open(dir) + if err != nil { + continue + } + names, err := handle.Readdirnames(-1) + safelyCloseFile(handle) + if err != nil { + continue + } + // discount the directory handle itself + return len(names) - 1 + } + return -1 +} + +func writeSampleFiles(t *testing.T, count int) []string { + t.Helper() + dir := t.TempDir() + filenames := make([]string, count) + for i := 0; i < count; i++ { + filename := filepath.Join(dir, "sample-"+strconv.Itoa(i)+".yml") + if err := os.WriteFile(filename, []byte("a: apple\n"), 0600); err != nil { + t.Fatalf("failed to write sample file: %v", err) + } + filenames[i] = filename + } + return filenames +} + +func discardingPrinter() Printer { + return NewPrinter(NewYamlEncoder(ConfiguredYamlPreferences), NewSinglePrinterWriter(bufio.NewWriter(io.Discard))) +} + +func assertNoLeakedFileDescriptors(t *testing.T, evaluate func(filenames []string) error) { + t.Helper() + InitExpressionParser() + + // os.File finalisers close leaked descriptors on collection, which would + // let a genuine leak pass unnoticed. + defer debug.SetGCPercent(debug.SetGCPercent(-1)) + + before := countOpenFileDescriptors() + if before < 0 { + t.Skip("file descriptors are not observable on this platform") + } + + if err := evaluate(writeSampleFiles(t, 50)); err != nil { + t.Fatalf("failed to evaluate files: %v", err) + } + + after := countOpenFileDescriptors() + if after > before { + t.Errorf("expected no additional open file descriptors, had %d before and %d after", before, after) + } +} + +func TestStreamEvaluatorClosesInputFiles(t *testing.T) { + assertNoLeakedFileDescriptors(t, func(filenames []string) error { + return NewStreamEvaluator().EvaluateFiles(".a", filenames, discardingPrinter(), NewYamlDecoder(ConfiguredYamlPreferences)) + }) +} + +func TestAllAtOnceEvaluatorClosesInputFiles(t *testing.T) { + assertNoLeakedFileDescriptors(t, func(filenames []string) error { + return NewAllAtOnceEvaluator().EvaluateFiles(".a", filenames, discardingPrinter(), NewYamlDecoder(ConfiguredYamlPreferences)) + }) +} + // plainWriter only implements io.Writer, so io.WriteString must fall back to Write. type plainWriter struct { buf bytes.Buffer