staging/hv: Fix memory leak of storvsc_dev_add()

PATCH In review Andre Bartke 2 participants 4 messages May 31, 2011 – Jun 1, 2011 Staging

lore

Share & tools

lore

Andre Bartke

author

2011-05-31 21:46 UTC #0 lore raw

PATCH 1/1 staging/hv: Fix memory leak of storvsc_dev_add()

stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.

Signed-off-by Andre Bartke <[email protected]>

---
 drivers/staging/hv/storvsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/staging/hv/storvsc.c +1 -0

index 06cd327..92b44f4 100644
@@ -414,6 +414,7 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->target_id = stor_device->target_id;
 
 cleanup:
+	free_stor_device(stor_device);
 	return ret;
 }

Signature · 1 line

1.7.5.2

KY Srinivasan

2011-06-01 12:28 UTC #1 lore raw

Quoted 24 lines

-----Original Message-----
From: [email protected] [mailto:devel- [email protected]] On Behalf Of Andre Bartke
Sent: Tuesday, May 31, 2011 5:46 PM
To: [email protected]
Cc: [email protected]; Andre Bartke; Haiyang Zhang; linux- [email protected]
Subject: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()
stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.
Signed-off-by Andre Bartke <[email protected]>
---
 drivers/staging/hv/storvsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/staging/hv/storvsc.c +0 -0
index 06cd327..92b44f4 100644

drivers/staging/hv/storvsc.c int storvsc_dev_add(struct hv_device *device, · line 418

Quoted 5 lines

drivers/staging/hv/storvsc.c +1 -0

@@ -414,6 +414,7 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->target_id = stor_device->target_id;
 
 cleanup:
+	free_stor_device(stor_device);
 	return ret;

Was this patch tested? Even the normal path falls through the cleanup label and obviously you should not be freeing the stor_device in that case. I have patches queued up to cleanup some of the unnecessary labels and meaningless return values that I will be sending soon.

Regards,

K. Y

Andre Bartke

author

2011-06-01 16:32 UTC #2 lore raw

Quoted 35 lines · On Wed, 1 Jun 2011 12:28:40 +0000 KY Srinivasan <[email protected]> wrote:

Quoted 24 lines
-----Original Message-----
From: [email protected] [mailto:devel- [email protected]] On Behalf Of Andre Bartke
Sent: Tuesday, May 31, 2011 5:46 PM
To: [email protected]
Cc: [email protected]; Andre Bartke; Haiyang Zhang; linux- [email protected]
Subject: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()
stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.
Signed-off-by Andre Bartke <[email protected]>
---
 drivers/staging/hv/storvsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/hv/storvsc.c index 06cd327..92b44f4 100644
drivers/staging/hv/storvsc.c int storvsc_dev_add(struct hv_device *device, · line 418
Quoted 5 lines
drivers/staging/hv/storvsc.c +1 -0
@@ -414,6 +414,7 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->target_id = stor_device->target_id;
 
 cleanup:
+	free_stor_device(stor_device);
 	return ret;
Was this patch tested? Even the normal path falls through the cleanup label and obviously you should not be freeing the stor_device in that case. I have patches queued up to cleanup some of the unnecessary labels and meaningless return values that I will be sending soon.

Quoted 6 lines

Regards,
K. Y

Right, I really messed up there, here is how it was supposed to be

Quoted 1 line

From 5fe8c601bae555b03257d0e7ea57e42a99c5e634 Mon Sep 17 00:00:00 2001

From: Andre Bartke <[email protected]>
Date: Wed, 1 Jun 2011 18:18:58 +0200
Subject: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()

stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.

Signed-off-by Andre Bartke <[email protected]>

---
 drivers/staging/hv/storvsc.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

drivers/staging/hv/storvsc.c +8 -4

index 06cd327..3d77c82 100644
@@ -392,10 +392,8 @@ int storvsc_dev_add(struct hv_device *device,
 
 	device_info = (struct storvsc_device_info *)additional_info;
 	stor_device = alloc_stor_device(device);
-	if (!stor_device) {
-		ret = -1;
-		goto cleanup;
-	}
+	if (!stor_device)
+		goto err;
 
 	/* Save the channel properties to our storvsc channel */

@@ -413,6 +411,12 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->path_id = stor_device->path_id;
 	device_info->target_id = stor_device->target_id;
 
+	goto cleanup;
+
+err:
+	ret = -1;
+	free_stor_device(stor_device);
+
 cleanup:
 	return ret;
 }

Signature · 1 line

1.7.5.2

KY Srinivasan

2011-06-01 18:03 UTC #3 lore raw

Quoted 101 lines

-----Original Message-----
From: Andre Bartke [mailto:[email protected]]
Sent: Wednesday, June 01, 2011 12:32 PM
To: KY Srinivasan
Cc: Andre Bartke; [email protected]; [email protected]; Haiyang Zhang; [email protected]
Subject: Re: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()
Quoted 35 lines · On Wed, 1 Jun 2011 12:28:40 +0000 KY Srinivasan <[email protected]> wrote:
Quoted 24 lines
-----Original Message-----
From: [email protected] [mailto:devel- [email protected]] On Behalf Of Andre Bartke
Sent: Tuesday, May 31, 2011 5:46 PM
To: [email protected]
Cc: [email protected]; Andre Bartke; Haiyang Zhang; linux- [email protected]
Subject: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()
stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.
Signed-off-by Andre Bartke <[email protected]>
---
 drivers/staging/hv/storvsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/hv/storvsc.c index 06cd327..92b44f4 100644
drivers/staging/hv/storvsc.c int storvsc_dev_add(struct hv_device *device, · line 418
Quoted 5 lines
drivers/staging/hv/storvsc.c +1 -0
@@ -414,6 +414,7 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->target_id = stor_device->target_id;
 
 cleanup:
+	free_stor_device(stor_device);
 	return ret;
Was this patch tested? Even the normal path falls through the cleanup label and obviously you should not be freeing the stor_device in that case. I have patches queued up to cleanup some of the unnecessary labels and meaningless return values that I will be sending soon.
Quoted 6 lines
Regards,
K. Y
Right, I really messed up there, here is how it was supposed to be
From 5fe8c601bae555b03257d0e7ea57e42a99c5e634 Mon Sep 17 00:00:00 2001
From: Andre Bartke <[email protected]>
Date: Wed, 1 Jun 2011 18:18:58 +0200
Subject: [PATCH] staging/hv: Fix memory leak of storvsc_dev_add()
stor_device is not freed in storvsc_dev_add() causing a memory leak in case of an allocation error.
Signed-off-by Andre Bartke <[email protected]>
---
 drivers/staging/hv/storvsc.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
drivers/staging/hv/storvsc.c +8 -5
index 06cd327..3d77c82 100644
@@ -392,10 +392,8 @@ int storvsc_dev_add(struct hv_device *device,
 
 	device_info = (struct storvsc_device_info *)additional_info;
 	stor_device = alloc_stor_device(device);
-	if (!stor_device) {
-		ret = -1;
-		goto cleanup;
-	}
+	if (!stor_device)
+		goto err;
 
 	/* Save the channel properties to our storvsc channel */
 
@@ -413,6 +411,12 @@ int storvsc_dev_add(struct hv_device *device,
 	device_info->path_id = stor_device->path_id;
 	device_info->target_id = stor_device->target_id;
 
+	goto cleanup;
+
+err:
+	ret = -1;
+	free_stor_device(stor_device);
+
 cleanup:
 	return ret;
 }
--

Quoted 2 lines

1.7.5.2

This could be cleaned up much better; we don't need to additional labels. As I said, earlier, I have patches queued up for this and a whole lot more. I will add your name to the patch that fixes this issue.

Regards,

K. Y