Merge pull request #226 from dell/mkdir-fix

Remove possible nil pointer dereference in mkdir

service/mount.go

@@ -1,7 +1,7 @@
 package service
 
 /*
- Copyright (c) 2019-2022 Dell Inc, or its subsidiaries.
+ Copyright (c) 2019-2023 Dell Inc, or its subsidiaries.
 
  Licensed under the Apache License, Version 2.0 (the "License");
  you may not use this file except in compliance with the License.
@@ -16,7 +16,9 @@ package service
  limitations under the License.
 */
 import (
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"strings"
 	"time"
@@ -177,21 +179,27 @@ func unpublishVolume(
 // mkdir creates the directory specified by path if needed.
 // return pair is a bool flag of whether dir was created, and an error
 func mkdir(ctx context.Context, path string) (bool, error) {
-	// Fetch log handler
+	_, log := GetLogger(ctx)
 	st, err := os.Stat(path)
-	if os.IsNotExist(err) {
-		if err := os.Mkdir(path, 0o750); err != nil {
-			logrus.WithField("dir", path).WithError(
-				err).Error("Unable to create dir")
-			return false, err
+	if err == nil {
+		if !st.IsDir() {
+			return false, fmt.Errorf("existing path is not a directory")
 		}
-		logrus.WithField("path", path).Debug("created directory")
-		return true, nil
+		return false, nil
 	}
-	if !st.IsDir() {
-		return false, fmt.Errorf("existing path is not a directory")
+	if !errors.Is(err, fs.ErrNotExist) {
+		log.WithField("dir", path).WithError(err).Error("Unable to stat dir")
+		return false, err
 	}
-	return false, nil
+
+	// Case when there is error and the error is fs.ErrNotExists.
+	if err := os.MkdirAll(path, 0o750); err != nil {
+		log.WithField("dir", path).WithError(err).Error("Unable to create dir")
+		return false, err
+	}
+
+	log.WithField("path", path).Debug("created directory")
+	return true, nil
 }
 
 func contains(list []string, item string) bool {

service/mount_test.go

@@ -0,0 +1,80 @@
+/*
+Copyright (c) 2019-2023 Dell Inc, or its subsidiaries.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package service
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// Split TestMkdir into separate tests to test each case
+func TestMkdirCreateDir(t *testing.T) {
+	ctx := context.Background()
+
+	// Make temp directory to use for testing
+	basepath, err := os.MkdirTemp("/tmp", "*")
+	assert.NoError(t, err)
+	defer os.RemoveAll(basepath)
+	path := basepath + "/test"
+
+	// Test creating a directory
+	created, err := mkdir(ctx, path)
+	assert.NoError(t, err)
+	assert.True(t, created)
+}
+
+func TestMkdirExistingDir(t *testing.T) {
+	ctx := context.Background()
+
+	// Make temp directory to use for testing
+	basepath, err := os.MkdirTemp("/tmp", "*")
+	assert.NoError(t, err)
+	defer os.RemoveAll(basepath)
+
+	path := basepath + "/test"
+
+	// Pre-create the directory
+	err = os.Mkdir(path, 0o755)
+	assert.NoError(t, err)
+
+	// Test creating an existing directory
+	created, err := mkdir(ctx, path)
+	assert.NoError(t, err)
+	assert.False(t, created)
+}
+
+func TestMkdirExistingFile(t *testing.T) {
+	ctx := context.Background()
+
+	// Make temp directory to use for testing
+	basepath, err := os.MkdirTemp("/tmp", "*")
+	assert.NoError(t, err)
+	defer os.RemoveAll(basepath)
+
+	path := basepath + "/file"
+	file, err := os.Create(path)
+	assert.NoError(t, err)
+	file.Close()
+
+	// Test creating a directory with an existing file path
+	created, err := mkdir(ctx, path)
+	assert.Error(t, err)
+	assert.False(t, created)
+}