Fix input file descriptor leak across multi-file evaluation

acceptance_tests/file-descriptors.sh

@@ -0,0 +1,32 @@
+#!/bin/bash
+
+setUp() {
+  rm test-fd-*.yml 2>/dev/null || true
+}
+
+tearDown() {
+  rm test-fd-*.yml 2>/dev/null || true
+}
+
+createInputFiles() {
+  local index
+  for index in {1..80}; do
+    ln -s examples/sample.yaml "test-fd-${index}.yml"
+  done
+}
+
+testEvalClosesInputFiles() {
+  createInputFiles
+
+  (ulimit -n 32; GOGC=off ./yq eval 'select(false)' test-fd-*.yml)
+  assertEquals 0 "$?"
+}
+
+testEvalAllClosesInputFiles() {
+  createInputFiles
+
+  (ulimit -n 32; GOGC=off ./yq eval-all 'select(false)' test-fd-*.yml)
+  assertEquals 0 "$?"
+}
+
+source ./scripts/shunit2

pkg/yqlib/all_at_once_evaluator.go

@@ -49,12 +49,7 @@ func (e *allAtOnceEvaluator) EvaluateFiles(expression string, filenames []string
 
 	var allDocuments = list.New()
 	for _, filename := range filenames {
-		reader, err := readStream(filename)
-		if err != nil {
-			return err
-		}
-
-		fileDocuments, err := readDocuments(reader, filename, fileIndex, decoder)
+		fileDocuments, err := readDocumentsFromFile(filename, fileIndex, decoder)
 		if err != nil {
 			return err
 		}
@@ -73,3 +68,13 @@ func (e *allAtOnceEvaluator) EvaluateFiles(expression string, filenames []string
 	}
 	return printer.PrintResults(matches)
 }
+
+func readDocumentsFromFile(filename string, fileIndex int, decoder Decoder) (*list.List, error) {
+	reader, err := readStream(filename)
+	if err != nil {
+		return nil, err
+	}
+	defer SafelyCloseReader(reader)
+
+	return readDocuments(reader, filename, fileIndex, decoder)
+}

pkg/yqlib/all_at_once_evaluator_test.go

@@ -2,6 +2,7 @@ package yqlib
 
 import (
 	"bufio"
+	"os"
 	"strings"
 	"testing"
 
@@ -76,3 +77,23 @@ func TestTomlDecoderCanBeReinitializedAcrossDocuments(t *testing.T) {
 	}
 	test.AssertResult(t, "Banana", secondDocuments.Front().Value.(*CandidateNode).Content[1].Value)
 }
+
+func TestReadDocumentsLeavesReaderOpen(t *testing.T) {
+	filename := t.TempDir() + "/input.yml"
+	if err := os.WriteFile(filename, []byte("a: apple\n"), 0600); err != nil {
+		t.Fatalf("failed to write input file: %v", err)
+	}
+
+	file, err := os.Open(filename)
+	if err != nil {
+		t.Fatalf("failed to open input file: %v", err)
+	}
+	defer file.Close()
+
+	if _, err := ReadDocuments(file, NewYamlDecoder(ConfiguredYamlPreferences)); err != nil {
+		t.Fatalf("failed to read documents: %v", err)
+	}
+	if _, err := file.Seek(0, 0); err != nil {
+		t.Fatalf("ReadDocuments closed the caller-owned reader: %v", err)
+	}
+}

pkg/yqlib/file_utils.go

@@ -62,6 +62,10 @@ func SafelyCloseReader(reader io.Reader) {
 	switch reader := reader.(type) {
 	case *os.File:
 		safelyCloseFile(reader)
+	case io.Closer:
+		if err := reader.Close(); err != nil {
+			log.Errorf("Error closing reader: %v", err)
+		}
 	}
 }
 

pkg/yqlib/front_matter_test.go

@@ -142,6 +142,7 @@ Some content
 	}
 	decoder := NewYamlDecoder(ConfiguredYamlPreferences)
 	docs, err := readDocuments(reader, tempFilename, 0, decoder)
+	SafelyCloseReader(reader)
 	if err != nil {
 		panic(err)
 	}

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,11 @@ func (s *streamEvaluator) EvaluateFiles(expression string, filenames []string, p
 	}
 
 	for _, filename := range filenames {
-		reader, err := readStream(filename)
-
-		if err != nil {
-			return err
-		}
-		processedDocs, err := s.Evaluate(filename, reader, node, printer, decoder)
+		processedDocs, err := s.evaluateFile(filename, node, printer, decoder)
 		if err != nil {
 			return err
 		}
 		totalProcessDocs = totalProcessDocs + processedDocs
-
-		switch reader := reader.(type) {
-		case *os.File:
-			safelyCloseFile(reader)
-		}
 	}
 
 	if totalProcessDocs == 0 {
@@ -75,6 +64,16 @@ func (s *streamEvaluator) EvaluateFiles(expression string, filenames []string, p
 	return nil
 }
 
+func (s *streamEvaluator) evaluateFile(filename string, node *ExpressionNode, printer Printer, decoder Decoder) (uint, error) {
+	reader, err := readStream(filename)
+	if err != nil {
+		return 0, err
+	}
+	defer SafelyCloseReader(reader)
+
+	return s.Evaluate(filename, reader, node, printer, decoder)
+}
+
 func (s *streamEvaluator) Evaluate(filename string, reader io.Reader, node *ExpressionNode, printer Printer, decoder Decoder) (uint, error) {
 	filename = resolveFilename(filename)
 

pkg/yqlib/utils.go

@@ -32,21 +32,23 @@ func resolveFilename(filename string) string {
 	return filename
 }
 
-func readStream(filename string) (io.Reader, error) {
-	var reader *bufio.Reader
+type readCloser struct {
+	io.Reader
+	io.Closer
+}
+
+func readStream(filename string) (io.ReadCloser, 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 io.NopCloser(bufio.NewReader(os.Stdin)), 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, err
+	}
+	return &readCloser{Reader: bufio.NewReader(file), Closer: file}, nil
 }
 
 func writeString(writer io.Writer, txt string) error {
@@ -71,10 +73,6 @@ func readDocuments(reader io.Reader, filename string, fileIndex int, decoder Dec
 		candidateNode, errorReading := decoder.Decode()
 
 		if errors.Is(errorReading, io.EOF) {
-			switch reader := reader.(type) {
-			case *os.File:
-				safelyCloseFile(reader)
-			}
 			return inputList, nil
 		} else if errorReading != nil {
 			return nil, fmt.Errorf("bad file '%v': %w", filename, errorReading)