From a8da3d679c1bd63155c00b0a35acfeb9baf5f0d4 Mon Sep 17 00:00:00 2001
From: Niels de Vos <ndevos@ibm.com>
Date: Wed, 17 Apr 2024 12:25:18 +0200
Subject: [PATCH] rbd: prevent calling rbdVolume.Destroy() after an error

It seems possible that the .Destroy() function is called on a nil
pointer. This would cause a panic in the node-plugin.

Depending on how far GenVolFromVolID() comes, the returned rbdVolume can
be connected. If an error occurs, the rbdVolume should not be connected
at all, so call the .Destroy() function in those cases too.

Fixes: #4548
Signed-off-by: Niels de Vos <ndevos@ibm.com>
---
 internal/rbd/nodeserver.go |  1 -
 internal/rbd/rbd_util.go   | 35 ++++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
index c203c82db85f..6382d99ae16f 100644
--- a/internal/rbd/nodeserver.go
+++ b/internal/rbd/nodeserver.go
@@ -198,7 +198,6 @@ func (ns *NodeServer) populateRbdVol(
 	} else {
 		rv, err = GenVolFromVolID(ctx, volID, cr, req.GetSecrets())
 		if err != nil {
-			rv.Destroy()
 			log.ErrorLog(ctx, "error generating volume %s: %v", volID, err)
 
 			return nil, status.Errorf(codes.Internal, "error generating volume %s: %v", volID, err)
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index e3852893295e..20ee2b0c24a6 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -1070,6 +1070,8 @@ func updateSnapshotDetails(rbdSnap *rbdSnapshot) error {
 }
 
 // generateVolumeFromVolumeID generates a rbdVolume structure from the provided identifier.
+// The returned rbdVolume is connected in case no error occurred, and should be
+// cleaned up with rbdVolume.Destroy().
 func generateVolumeFromVolumeID(
 	ctx context.Context,
 	volumeID string,
@@ -1116,6 +1118,13 @@ func generateVolumeFromVolumeID(
 	if err != nil {
 		return rbdVol, err
 	}
+	// in case of any error call Destroy for cleanup.
+	defer func() {
+		if err != nil {
+			rbdVol.Destroy()
+		}
+	}()
+
 	rbdVol.JournalPool = rbdVol.Pool
 
 	imageAttributes, err := j.GetImageAttributes(
@@ -1163,6 +1172,8 @@ func generateVolumeFromVolumeID(
 
 // GenVolFromVolID generates a rbdVolume structure from the provided identifier, updating
 // the structure with elements from on-disk image metadata as well.
+// The returned rbdVolume is connected in case no error occurred, and should be
+// cleaned up with rbdVolume.Destroy().
 func GenVolFromVolID(
 	ctx context.Context,
 	volumeID string,
@@ -1185,17 +1196,27 @@ func GenVolFromVolID(
 		!errors.Is(err, ErrImageNotFound) {
 		return vol, err
 	}
+	defer func() {
+		if err != nil {
+			vol.Destroy()
+		}
+	}()
 
 	// Check clusterID mapping exists
-	mapping, mErr := util.GetClusterMappingInfo(vi.ClusterID)
-	if mErr != nil {
-		return vol, mErr
+	mapping, err := util.GetClusterMappingInfo(vi.ClusterID)
+	if err != nil {
+		return vol, err
 	}
 	if mapping != nil {
-		rbdVol, vErr := generateVolumeFromMapping(ctx, mapping, volumeID, vi, cr, secrets)
-		if !errors.Is(vErr, util.ErrKeyNotFound) && !errors.Is(vErr, util.ErrPoolNotFound) &&
-			!errors.Is(vErr, ErrImageNotFound) {
-			return rbdVol, vErr
+		// create a new volume based on the mapping, cleanup the old volume
+		if vol != nil {
+			vol.Destroy()
+		}
+
+		vol, err = generateVolumeFromMapping(ctx, mapping, volumeID, vi, cr, secrets)
+		if !errors.Is(err, util.ErrKeyNotFound) && !errors.Is(err, util.ErrPoolNotFound) &&
+			!errors.Is(err, ErrImageNotFound) {
+			return vol, err
 		}
 	}