From cb7b707457a3c963a3f16d3075b4cbea2989535b Mon Sep 17 00:00:00 2001 From: MsfPablo <129399053+MsfPablo@users.noreply.github.com> Date: Wed, 19 Aug 2026 06:17:53 +0200 Subject: [PATCH] Close input files after processing each one (#2796) (#2808) readStream opened an *os.File but returned a *bufio.Reader, so the *os.File type switch in streamEvaluator.EvaluateFiles never matched and no input file was ever closed. allAtOnceEvaluator.EvaluateFiles did not attempt to close them at all. Descriptors were only released at process exit or by the os.File finaliser, so a multi-file run could exhaust the file descriptor limit. readStream now returns an explicit cleanup function alongside the reader, which both evaluators call once the file has been processed. Co-authored-by: Pablo Garcia Co-authored-by: Mike Farah Co-authored-by: Cursor --- pkg/yqlib/all_at_once_evaluator.go | 3 +- pkg/yqlib/front_matter_test.go | 3 +- pkg/yqlib/stream_evaluator.go | 9 +--- pkg/yqlib/utils.go | 25 +++++----- pkg/yqlib/utils_test.go | 78 ++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 22 deletions(-) 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