Fix segfault in disk space check

CHANGELOG.md

@@ -21,6 +21,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Added a progress indicator for the running checks.
   [conjurinc/conjur-preflight#24](https://github.com/conjurinc/conjur-preflight/pull/24)
 
+### Fixed
+- Previously, if the user running `conjur-preflight` has insufficient permission
+  to access a partition mountpoint, then the check would result in a segfault.
+  Now, this logs a warning and skips the mountpoint in the report.
+  [conjurinc/conjur-preflight#28](https://github.com/conjurinc/conjur-preflight/pull/28)
+
 ## [0.2.0] - 2023-01-20
 
 ### Added

pkg/checks/cpu.go

@@ -7,14 +7,17 @@ import (
 	"github.com/conjurinc/conjur-preflight/pkg/framework"
 )
 
+// Cpu collects inspection information on the host machines CPU cores and
+// architecture
 type Cpu struct {
 }
 
-// Describe provides a textual description of what this check gathers info on
+// Describe provides a textual description of the info this check gathers
 func (*Cpu) Describe() string {
 	return "CPU"
 }
 
+// Run executes the CPU inspection checks
 func (cpu *Cpu) Run() <-chan []framework.CheckResult {
 	future := make(chan []framework.CheckResult)
 

pkg/checks/cpu_test.go

@@ -1,16 +1,15 @@
-package checks_test
+package checks
 
 import (
 	"regexp"
 	"testing"
 
-	"github.com/conjurinc/conjur-preflight/pkg/checks"
 	"github.com/conjurinc/conjur-preflight/pkg/framework"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestCpuRun(t *testing.T) {
-	testCheck := &checks.Cpu{}
+	testCheck := &Cpu{}
 	resultChan := testCheck.Run()
 	results := <-resultChan
 

pkg/checks/disk/iops.go

@@ -23,6 +23,8 @@ type IopsCheck struct {
 	fioNewJob func(string, []string) fio.Executable
 }
 
+var getWorkingDirectory func() (string, error) = os.Getwd
+
 // NewIopsCheck instantiates an Iops check with the default dependencies
 func NewIopsCheck(debug bool) *IopsCheck {
 	return &IopsCheck{
@@ -91,7 +93,11 @@ func fioReadIopsResult(job *fio.JobResult) framework.CheckResult {
 	}
 
 	// Format title
-	path, _ := os.Getwd()
+	path, err := getWorkingDirectory()
+	if err != nil {
+		log.Debug("Unable to get working directory: %s", err)
+		path = "working directory"
+	}
 	titleStr := fmt.Sprintf("FIO - Read IOPs (%s)", path)
 
 	// Format value
@@ -119,7 +125,11 @@ func fioWriteIopsResult(job *fio.JobResult) framework.CheckResult {
 	}
 
 	// Format title
-	path, _ := os.Getwd()
+	path, err := getWorkingDirectory()
+	if err != nil {
+		log.Debug("Unable to get working directory: %s", err)
+		path = "working directory"
+	}
 	titleStr := fmt.Sprintf("FIO - Write IOPs (%s)", path)
 
 	// Format value

pkg/checks/disk/iops_test.go

@@ -94,6 +94,32 @@ func TestIopsWithNoJobs(t *testing.T) {
 	assert.Equal(t, "No job results returned by 'fio'", results[0].Message)
 }
 
+func TestIopsWithWorkingDirectoryError(t *testing.T) {
+	// Double the working directory function to simulate it failing with an error
+	originalWorkingDirectoryFunc := getWorkingDirectory
+	getWorkingDirectory = failedWorkingDir
+	defer func() {
+		getWorkingDirectory = originalWorkingDirectoryFunc
+	}()
+
+	testCheck := &IopsCheck{
+		debug:     true,
+		fioNewJob: newSuccessfulIopsFioJob,
+	}
+	resultChan := testCheck.Run()
+	results := <-resultChan
+
+	assert.Equal(
+		t,
+		2,
+		len(results),
+		"There are read and write IOPs results present",
+	)
+
+	assertReadIopsResult(t, results[0], framework.STATUS_INFO)
+	assertWriteIopsResult(t, results[1], framework.STATUS_INFO)
+}
+
 func assertReadIopsResult(
 	t *testing.T,
 	result framework.CheckResult,

pkg/checks/disk/latency.go

@@ -2,10 +2,10 @@ package disk
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/conjurinc/conjur-preflight/pkg/checks/disk/fio"
 	"github.com/conjurinc/conjur-preflight/pkg/framework"
+	"github.com/conjurinc/conjur-preflight/pkg/log"
 )
 
 // LatencyCheck is a pre-flight check to report the read, write, and sync
@@ -90,7 +90,11 @@ func fioReadLatencyResult(jobResult *fio.JobResult) framework.CheckResult {
 		status = framework.STATUS_WARN
 	}
 
-	path, _ := os.Getwd()
+	path, err := getWorkingDirectory()
+	if err != nil {
+		log.Debug("Unable to get working directory: %s", err)
+		path = "working directory"
+	}
 
 	return framework.CheckResult{
 		Title:  fmt.Sprintf("FIO - Read Latency (99%%, %s)", path),
@@ -110,7 +114,11 @@ func fioWriteLatencyResult(jobResult *fio.JobResult) framework.CheckResult {
 		status = framework.STATUS_WARN
 	}
 
-	path, _ := os.Getwd()
+	path, err := getWorkingDirectory()
+	if err != nil {
+		log.Debug("Unable to get working directory: %s", err)
+		path = "working directory"
+	}
 
 	return framework.CheckResult{
 		Title:  fmt.Sprintf("FIO - Write Latency (99%%, %s)", path),
@@ -130,7 +138,11 @@ func fioSyncLatencyResult(jobResult *fio.JobResult) framework.CheckResult {
 		status = framework.STATUS_WARN
 	}
 
-	path, _ := os.Getwd()
+	path, err := getWorkingDirectory()
+	if err != nil {
+		log.Debug("Unable to get working directory: %s", err)
+		path = "working directory"
+	}
 
 	return framework.CheckResult{
 		Title:  fmt.Sprintf("FIO - Sync Latency (99%%, %s)", path),

pkg/checks/disk/latency_test.go

@@ -3,6 +3,7 @@
 package disk
 
 import (
+	"errors"
 	"regexp"
 	"testing"
 
@@ -71,6 +72,27 @@ func TestLatencyWithNoJobs(t *testing.T) {
 	assert.Equal(t, "No job results returned by 'fio'", results[0].Message)
 }
 
+func TestLatencyWithWorkingDirectoryError(t *testing.T) {
+	// Double the working directory function to simulate it failing with an error
+	originalWorkingDirectoryFunc := getWorkingDirectory
+	getWorkingDirectory = failedWorkingDir
+	defer func() {
+		getWorkingDirectory = originalWorkingDirectoryFunc
+	}()
+
+	testCheck := &LatencyCheck{
+		fioNewJob: newSuccessfulLatencyFioJob,
+	}
+	resultChan := testCheck.Run()
+	results := <-resultChan
+
+	assert.Equal(t, 3, len(results), "There are disk latency results present")
+
+	assertReadLatencyResult(t, results[0], framework.STATUS_INFO)
+	assertWriteLatencyResult(t, results[1], framework.STATUS_INFO)
+	assertSyncLatencyResult(t, results[2], framework.STATUS_INFO)
+}
+
 func assertReadLatencyResult(
 	t *testing.T,
 	result framework.CheckResult,
@@ -187,3 +209,7 @@ func newPoorLatencyPerformanceFioJob(
 		},
 	}
 }
+
+func failedWorkingDir() (string, error) {
+	return "", errors.New("working directory error")
+}

pkg/checks/disk/space.go

@@ -4,30 +4,58 @@ import (
 	"fmt"
 
 	"github.com/conjurinc/conjur-preflight/pkg/framework"
+	"github.com/conjurinc/conjur-preflight/pkg/log"
 	"github.com/dustin/go-humanize"
 	"github.com/shirou/gopsutil/v3/disk"
 )
 
+// Aliasing our external dependencies like this allows to swap them out for
+// testing
+var getPartitions func(all bool) ([]disk.PartitionStat, error) = disk.Partitions
+var getUsage func(path string) (*disk.UsageStat, error) = disk.Usage
+
 // SpaceCheck reports on the available partitions and devices on the current
 // machine, as well as their available disk space.
-type SpaceCheck struct {
-}
+type SpaceCheck struct{}
 
 // Describe provides a textual description of what this check gathers info on
 func (*SpaceCheck) Describe() string {
 	return "disk capacity"
 }
 
 // Run executes the disk checks and returns their results
-func (*SpaceCheck) Run() <-chan []framework.CheckResult {
+func (spaceCheck *SpaceCheck) Run() <-chan []framework.CheckResult {
 	future := make(chan []framework.CheckResult)
 
 	go func() {
-		partitions, _ := disk.Partitions(true)
+		partitions, err := getPartitions(true)
+		// If we can't list the partitions, we exit early with the failure message
+		if err != nil {
+			log.Debug("Unable to list disk partitions: %s", err)
+			future <- []framework.CheckResult{
+				{
+					Title:  "Error",
+					Status: framework.STATUS_ERROR,
+					Value:  fmt.Sprintf("%s", err),
+				},
+			}
+
+			return
+		}
+
 		results := []framework.CheckResult{}
 
 		for _, partition := range partitions {
-			usage, _ := disk.Usage(partition.Mountpoint)
+			usage, err := getUsage(partition.Mountpoint)
+			if err != nil {
+				log.Debug(
+					"Unable to collect disk usage for '%s': %s",
+					partition.Mountpoint,
+					err,
+				)
+				continue
+			}
+
 			if usage.Total == 0 {
 				continue
 			}

pkg/checks/disk/space_test.go

@@ -1,16 +1,17 @@
-package disk_test
+package disk
 
 import (
+	"errors"
 	"regexp"
 	"testing"
 
-	"github.com/conjurinc/conjur-preflight/pkg/checks/disk"
 	"github.com/conjurinc/conjur-preflight/pkg/framework"
+	"github.com/shirou/gopsutil/v3/disk"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestSpaceCheck(t *testing.T) {
-	testCheck := &disk.SpaceCheck{}
+	testCheck := &SpaceCheck{}
 	resultChan := testCheck.Run()
 	results := <-resultChan
 
@@ -32,3 +33,45 @@ func TestSpaceCheck(t *testing.T) {
 		)
 	}
 }
+
+func TestPartitionListError(t *testing.T) {
+	// Double the usage function to simulate an error
+	originalPartitionsFunc := getPartitions
+	getPartitions = failedPartitionsFunc
+	defer func() {
+		getPartitions = originalPartitionsFunc
+	}()
+
+	testCheck := &SpaceCheck{}
+	resultChan := testCheck.Run()
+	results := <-resultChan
+
+	assert.Len(t, results, 1)
+
+	errResult := results[0]
+	assert.Equal(t, "Error", errResult.Title)
+	assert.Equal(t, "test partitions failure", errResult.Value)
+}
+
+func TestDiskUsageError(t *testing.T) {
+	// Double the usage function to simulate an error
+	originalUsageFunc := getUsage
+	getUsage = failedUsageFunc
+	defer func() {
+		getUsage = originalUsageFunc
+	}()
+
+	testCheck := &SpaceCheck{}
+	resultChan := testCheck.Run()
+	results := <-resultChan
+
+	assert.Empty(t, results)
+}
+
+func failedPartitionsFunc(all bool) ([]disk.PartitionStat, error) {
+	return nil, errors.New("test partitions failure")
+}
+
+func failedUsageFunc(path string) (*disk.UsageStat, error) {
+	return nil, errors.New("test usage failure")
+}

pkg/checks/follower_test.go

@@ -1,15 +1,14 @@
-package checks_test
+package checks
 
 import (
 	"testing"
 
-	"github.com/conjurinc/conjur-preflight/pkg/checks"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestFollowerRun(t *testing.T) {
 	t.Setenv("MASTER_HOSTNAME", "http://example.com")
-	testCheck := &checks.Follower{}
+	testCheck := &Follower{}
 	resultChan := testCheck.Run()
 	results := <-resultChan