From e266ee69cea06e5c971af9005717a7c652866a08 Mon Sep 17 00:00:00 2001
From: Joe Lanford <joe.lanford@gmail.com>
Date: Mon, 2 Jan 2023 15:03:34 -0500
Subject: [PATCH] fix: don't panic when closing registry querier

When certain errors occur loading a registry.Querier, it is possible for
a Querier to be returned with a nil cache. This causes a panic when
Close is later called on the Querier.

This commit refactors the behavior such that:
1. If any error occurs creating a Querier, any necessary cleanup happens
   and a nil Querier is returned.
2. Querier.Close checks for a nil cache before attempting a close of the
   Querier's underlying cache.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
---
 cmd/opm/serve/serve.go     |  2 +-
 pkg/registry/query.go      | 34 ++++++++++++++++++++------------
 pkg/registry/query_test.go | 40 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/cmd/opm/serve/serve.go b/cmd/opm/serve/serve.go
index 633ad9ef3..3b9c081d4 100644
--- a/cmd/opm/serve/serve.go
+++ b/cmd/opm/serve/serve.go
@@ -103,10 +103,10 @@ func (s *serve) run(ctx context.Context) error {
 	s.logger = s.logger.WithFields(logrus.Fields{"configs": s.configDir, "port": s.port})
 
 	store, err := registry.NewQuerierFromFS(os.DirFS(s.configDir), s.cacheDir)
-	defer store.Close()
 	if err != nil {
 		return err
 	}
+	defer store.Close()
 	if s.cacheOnly {
 		return nil
 	}
diff --git a/pkg/registry/query.go b/pkg/registry/query.go
index 8a170b994..5e4f74bcf 100644
--- a/pkg/registry/query.go
+++ b/pkg/registry/query.go
@@ -27,7 +27,12 @@ type Querier struct {
 }
 
 func (q Querier) Close() error {
-	return q.cache.close()
+	if q.cache != nil {
+		if err := q.cache.close(); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 type apiBundleKey struct {
@@ -47,26 +52,22 @@ func (s *SliceBundleSender) Send(b *api.Bundle) error {
 var _ GRPCQuery = &Querier{}
 
 func NewQuerierFromFS(fbcFS fs.FS, cacheDir string) (*Querier, error) {
-	q := &Querier{}
-	var err error
-	q.cache, err = newCache(cacheDir, &fbcCacheModel{
+	cache, err := newCache(cacheDir, &fbcCacheModel{
 		FBC:   fbcFS,
 		Cache: os.DirFS(cacheDir),
 	})
 	if err != nil {
-		return q, err
+		return nil, err
 	}
-	return q, nil
+	return &Querier{cache: cache}, nil
 }
 
 func NewQuerier(m model.Model) (*Querier, error) {
-	q := &Querier{}
-	var err error
-	q.cache, err = newCache("", &nonDigestableModel{Model: m})
+	cache, err := newCache("", &nonDigestableModel{Model: m})
 	if err != nil {
-		return q, err
+		return nil, err
 	}
-	return q, nil
+	return &Querier{cache: cache}, nil
 }
 
 func (q Querier) loadAPIBundle(k apiBundleKey) (*api.Bundle, error) {
@@ -414,7 +415,13 @@ func newCache(baseDir string, model digestableModel) (*cache, error) {
 	if err != nil {
 		return nil, err
 	}
-	return qc, qc.load(model)
+	if err := qc.load(model); err != nil {
+		// try to clean up after ourselves by closing the cache (by cleaning
+		// up any temporary files/directories) before returning the error.
+		_ = qc.close()
+		return nil, err
+	}
+	return qc, nil
 }
 
 func (qc cache) close() error {
@@ -430,6 +437,9 @@ func newEphemeralCache() (*cache, error) {
 		return nil, err
 	}
 	if err := os.MkdirAll(filepath.Join(baseDir, "cache"), cachePermissionDir); err != nil {
+		// try to clean up after ourselves, so we don't leave a directory around
+		// when returning an error.
+		_ = os.RemoveAll(baseDir)
 		return nil, err
 	}
 	return &cache{
diff --git a/pkg/registry/query_test.go b/pkg/registry/query_test.go
index a732de741..4c2971ba8 100644
--- a/pkg/registry/query_test.go
+++ b/pkg/registry/query_test.go
@@ -3,6 +3,7 @@ package registry
 import (
 	"context"
 	"io/fs"
+	"os"
 	"testing"
 	"testing/fstest"
 
@@ -248,6 +249,45 @@ func TestQuerier_BadBundleRaisesError(t *testing.T) {
 	})
 }
 
+func TestQuerier_NewQuerierFromFS_FailMkdir(t *testing.T) {
+	origTmpDir, tmpDirSet := os.LookupEnv("TMPDIR")
+	tmpDir := t.TempDir()
+	os.Chmod(tmpDir, 0444)
+	require.NoError(t, os.Setenv("TMPDIR", tmpDir))
+	defer func() {
+		if tmpDirSet {
+			require.NoError(t, os.Setenv("TMPDIR", origTmpDir))
+		} else {
+			os.Unsetenv("TMPDIR")
+		}
+	}()
+
+	q, err := NewQuerierFromFS(validFS, "")
+	require.Nil(t, q)
+	require.Error(t, err)
+}
+
+func TestQuerier_NewQuerierFromFS_FailCleanup(t *testing.T) {
+	origTmpDir, tmpDirSet := os.LookupEnv("TMPDIR")
+	tmpDir := t.TempDir()
+	require.NoError(t, os.Setenv("TMPDIR", tmpDir))
+	defer func() {
+		if tmpDirSet {
+			require.NoError(t, os.Setenv("TMPDIR", origTmpDir))
+		} else {
+			os.Unsetenv("TMPDIR")
+		}
+	}()
+
+	q, err := NewQuerierFromFS(badBundleFS, "")
+	require.Nil(t, q)
+	require.Error(t, err)
+
+	tmpDirEntries, err := os.ReadDir(tmpDir)
+	require.Len(t, tmpDirEntries, 0)
+	require.NoError(t, err)
+}
+
 func genTestQueriers(t *testing.T, fbcFS fs.FS) []*Querier {
 	t.Helper()