diff mbox

[5/6,v2] rbd: fix leak of format 2 snapshot names

Message ID 517ABC06.7080200@inktank.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Elder April 26, 2013, 5:40 p.m. UTC
(I found a bug in this (which masked another bug).  Here is
version 2.  I have corrected it and updated the various
branches that depended on it.)


When the snapshot context for an rbd device gets updated (or the
initial one is recorded) a a list of snapshot structures is created
to represent them, one entry per snapshot.  Each entry includes a
dynamically-allocated copy of the snapshot name.

Currently the name is allocated in rbd_snap_create(), as a duplicate
of the passed-in name.

For format 1 images, the snapshot name provided is just a pointer to
an existing name.  But for format 2 images, the passed-in name is
already dynamically allocated, and in the the process of duplicating
it here we are leaking the passed-in name.

Fix this by dynamically allocating the name for format 1 snapshots
also, and then stop allocating a duplicate in rbd_snap_create().

Change rbd_dev_v1_snap_info() so none of its parameters is
side-effected unless it's going to return success.

This is part of:
    http://tracker.ceph.com/issues/4803

Signed-off-by: Alex Elder <elder@inktank.com>
---
v2: kill an errant semicolon; use loop iterator rather than "which"

 drivers/block/rbd.c |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
diff mbox

Patch

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 916741b..c15bb3f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3427,46 +3427,44 @@  static struct rbd_snap *rbd_snap_create(struct
rbd_device *rbd_dev,
 						u64 snap_features)
 {
 	struct rbd_snap *snap;
-	int ret;

 	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
 	if (!snap)
 		return ERR_PTR(-ENOMEM);

-	ret = -ENOMEM;
-	snap->name = kstrdup(snap_name, GFP_KERNEL);
-	if (!snap->name)
-		goto err;
-
+	snap->name = snap_name;
 	snap->id = snap_id;
 	snap->size = snap_size;
 	snap->features = snap_features;

 	return snap;
-
-err:
-	kfree(snap->name);
-	kfree(snap);
-
-	return ERR_PTR(ret);
 }

+/*
+ * Returns a dynamically-allocated snapshot name if successful, or a
+ * pointer-coded error otherwise.
+ */
 static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which,
 		u64 *snap_size, u64 *snap_features)
 {
 	char *snap_name;
+	int i;

 	rbd_assert(which < rbd_dev->header.snapc->num_snaps);

-	*snap_size = rbd_dev->header.snap_sizes[which];
-	*snap_features = 0;	/* No features for v1 */
-
 	/* Skip over names until we find the one we are looking for */

 	snap_name = rbd_dev->header.snap_names;
-	while (which--)
+	for (i = 0; i < which; i++)
 		snap_name += strlen(snap_name) + 1;

+	snap_name = kstrdup(snap_name, GFP_KERNEL);
+	if (!snap_name)
+		return ERR_PTR(-ENOMEM);
+
+	*snap_size = rbd_dev->header.snap_sizes[which];
+	*snap_features = 0;	/* No features for v1 */
+
 	return snap_name;
 }