From b35dd80ddfaa693675f0c7306af4dd14d7e98b81 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Mon, 20 Nov 2023 16:04:26 -0500 Subject: [PATCH] server: fix error handling in status server GetFiles Previously, we ignored errors returned by the stat file call in status server's `GetFiles`. This can lead to a panic if the stat call returns an error as we dereference the nil file info object later on without any guards. This commit patches the nil pointer dereference by returning any error encountered by stat immediately. Note that in the future we should probably change this to just ignore the current file path and continue with the rest of the files in the list. However, this is a good first patch to minimize backported code changes. Fixes: #114453 Release note (bug fix): GetFiles should not cause a nil pointer dereference. --- pkg/server/BUILD.bazel | 1 + pkg/server/get_local_files_test.go | 99 +++++++++++++++++++++++ pkg/server/status.go | 3 +- pkg/server/status_local_file_retrieval.go | 13 ++- 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 pkg/server/get_local_files_test.go diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index dad69ab5269e..54f4fd060856 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -433,6 +433,7 @@ go_test( "critical_nodes_test.go", "distsql_flows_test.go", "drain_test.go", + "get_local_files_test.go", "graphite_test.go", "grpc_gateway_test.go", "helpers_test.go", diff --git a/pkg/server/get_local_files_test.go b/pkg/server/get_local_files_test.go new file mode 100644 index 000000000000..f6eed65e7794 --- /dev/null +++ b/pkg/server/get_local_files_test.go @@ -0,0 +1,99 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package server + +import ( + "os" + "path/filepath" + "testing" + + "github.com/cockroachdb/cockroach/pkg/server/serverpb" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +// TestGetLocalFiles tests that retrieving local heap and goroutine profiles +// returns and reads files when they exist and returns the expected error +// when the file on encountered errors. +// Note that there is more extensive end to end testing of this done in +// pkg/server/storage_api/files_test.go. This test was created to test the +// error handling in response to #114453. +func TestGetLocalFiles(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + // We'll read a file successfully first to ensure the read logic is working. + tempDir, cleanupFn := testutils.TempDir(t) + defer cleanupFn() + + testHeapDir := filepath.Join(tempDir, "heap_profiles") + if err := os.MkdirAll(testHeapDir, os.ModePerm); err != nil { + t.Fatal(err) + } + + t.Run("read files successfully", func(t *testing.T) { + testHeapFile := filepath.Join(testHeapDir, "im-in-the-heap-dir") + if err := os.WriteFile(testHeapFile, []byte("heap stuff"), 0644); err != nil { + t.Fatal(err) + } + req := &serverpb.GetFilesRequest{ + NodeId: "local", Type: serverpb.FileType_HEAP, Patterns: []string{"*"}} + res, err := getLocalFiles(req, testHeapDir, "", os.Stat, os.ReadFile) + require.NoError(t, err) + require.Equal(t, 1, len(res.Files)) + require.Equal(t, "im-in-the-heap-dir", res.Files[0].Name) + }) + + t.Run("stat file error", func(t *testing.T) { + testHeapFile := filepath.Join(testHeapDir, "im-in-the-heap-dir") + if err := os.WriteFile(testHeapFile, []byte("heap stuff"), 0644); err != nil { + t.Fatal(err) + } + // Test that getLocalFiles returns an error when the file cannot be read with stat. + statFileWithErr := func(name string) (os.FileInfo, error) { + return nil, errors.Errorf("stat error") + } + req := &serverpb.GetFilesRequest{ + NodeId: "local", Type: serverpb.FileType_HEAP, Patterns: []string{"*"}} + _, err := getLocalFiles(req, testHeapDir, "", statFileWithErr, os.ReadFile) + require.ErrorContains(t, err, "stat error") + }) + + t.Run("read file error", func(t *testing.T) { + testHeapFile := filepath.Join(testHeapDir, "im-in-the-heap-dir") + if err := os.WriteFile(testHeapFile, []byte("heap stuff"), 0644); err != nil { + t.Fatal(err) + } + // Test that getLocalFiles returns an error when the file cannot be read. + readFileWithErr := func(name string) ([]byte, error) { + return nil, errors.Errorf("read error") + } + req := &serverpb.GetFilesRequest{ + NodeId: "local", Type: serverpb.FileType_HEAP, Patterns: []string{"*"}} + _, err := getLocalFiles(req, testHeapDir, "", os.Stat, readFileWithErr) + require.ErrorContains(t, err, "read error") + }) + + t.Run("dirs not implemented", func(t *testing.T) { + req := &serverpb.GetFilesRequest{ + NodeId: "local", Type: serverpb.FileType_HEAP, Patterns: []string{"*"}} + _, err := getLocalFiles(req, "", "nonexistent", os.Stat, os.ReadFile) + require.ErrorContains(t, err, "dump directory not configured") + + req = &serverpb.GetFilesRequest{ + NodeId: "local", Type: serverpb.FileType_GOROUTINES, Patterns: []string{"*"}} + _, err = getLocalFiles(req, "nonexistent", "", os.Stat, os.ReadFile) + require.ErrorContains(t, err, "dump directory not configured") + }) +} diff --git a/pkg/server/status.go b/pkg/server/status.go index 72bb2c7930c6..1559e1cc805d 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -1194,7 +1194,8 @@ func (s *statusServer) GetFiles( return status.GetFiles(ctx, req) } - return getLocalFiles(req, s.sqlServer.cfg.HeapProfileDirName, s.sqlServer.cfg.GoroutineDumpDirName) + return getLocalFiles(req, s.sqlServer.cfg.HeapProfileDirName, s.sqlServer.cfg.GoroutineDumpDirName, + os.Stat, os.ReadFile) } // checkFilePattern checks if a pattern is acceptable for the GetFiles call. diff --git a/pkg/server/status_local_file_retrieval.go b/pkg/server/status_local_file_retrieval.go index e390fd1d3d4e..8bd44cc020b1 100644 --- a/pkg/server/status_local_file_retrieval.go +++ b/pkg/server/status_local_file_retrieval.go @@ -127,7 +127,11 @@ func stacksLocal(req *serverpb.StacksRequest) (*serverpb.JSONResponse, error) { // getLocalFiles retrieves the requested files for the local node. This method // returns a gRPC error to the caller. func getLocalFiles( - req *serverpb.GetFilesRequest, heapProfileDirName string, goroutineDumpDirName string, + req *serverpb.GetFilesRequest, + heapProfileDirName string, + goroutineDumpDirName string, + statFileFn func(string) (os.FileInfo, error), + readFileFn func(string) ([]byte, error), ) (*serverpb.GetFilesResponse, error) { var dir string switch req.Type { @@ -154,10 +158,13 @@ func getLocalFiles( } for _, path := range filepaths { - fileinfo, _ := os.Stat(path) + fileinfo, err := statFileFn(path) + if err != nil { + return nil, status.Errorf(codes.Internal, err.Error()) + } var contents []byte if !req.ListOnly { - contents, err = os.ReadFile(path) + contents, err = readFileFn(path) if err != nil { return nil, status.Errorf(codes.Internal, err.Error()) }