[PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[Ronit Halder]

In dgap_parsefile() char pointers are set with kstrdup()
without checking that some string is allocated to that
char pointer before. This patch frees the memory if already allocated
and then set the poniter with kstrdup().

Signed-off-by: Ronit halder <ronit.crj@gmail.com>
---
 drivers/staging/dgap/dgap.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index e17bde7..64f6149 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -672,6 +672,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.portstr);
 			p->u.board.portstr = kstrdup(s, GFP_KERNEL);
 			if (kstrtol(s, 0, &p->u.board.port)) {
 				pr_err("bad number for IO port");
@@ -690,6 +691,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.addrstr);
 			p->u.board.addrstr = kstrdup(s, GFP_KERNEL);
 			if (kstrtoul(s, 0, &p->u.board.addr)) {
 				pr_err("bad number for memory address");
@@ -708,6 +710,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.pcibusstr);
 			p->u.board.pcibusstr = kstrdup(s, GFP_KERNEL);
 			if (kstrtoul(s, 0, &p->u.board.pcibus)) {
 				pr_err("bad number for pci bus");
@@ -719,6 +722,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.pcislotstr);
 			p->u.board.pcislotstr = kstrdup(s, GFP_KERNEL);
 			if (kstrtoul(s, 0, &p->u.board.pcislot)) {
 				pr_err("bad number for pci slot");
@@ -737,6 +741,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.method);
 			p->u.board.method = kstrdup(s, GFP_KERNEL);
 			p->u.board.v_method = 1;
 			break;
@@ -751,6 +756,7 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
+			kfree(p->u.board.status);
 			p->u.board.status = kstrdup(s, GFP_KERNEL);
 			break;
 
@@ -800,13 +806,15 @@ static int dgap_parsefile(char **in)
 				pr_err("unexpected end of file");
 				return -1;
 			}
-
+			kfree(p->u.board.status);
 			p->u.board.status = kstrdup(s, GFP_KERNEL);
 
 			if (p->type == CNODE) {
+				kfree(p->u.conc.id);
 				p->u.conc.id = kstrdup(s, GFP_KERNEL);
 				p->u.conc.v_id = 1;
 			} else if (p->type == MNODE) {
+				kfree(p->u.module.id);
 				p->u.module.id = kstrdup(s, GFP_KERNEL);
 				p->u.module.v_id = 1;
 			} else {
@@ -1003,6 +1011,7 @@ static int dgap_parsefile(char **in)
 					pr_err("unexpected end of file");
 					return -1;
 				}
+				kfree(p->u.line.cable);
 				p->u.line.cable = kstrdup(s, GFP_KERNEL);
 				p->u.line.v_cable = 1;
 			}
@@ -1044,6 +1053,7 @@ static int dgap_parsefile(char **in)
 					pr_err("unexpected end of file");
 					return -1;
 				}
+				kfree(p->u.conc.connect);
 				p->u.conc.connect = kstrdup(s, GFP_KERNEL);
 				p->u.conc.v_connect = 1;
 			}
-- 
2.4.0.GIT

Re: [PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[Dan Carpenter]

On Wed, Sep 30, 2015 at 11:39:45AM +0530, Ronit Halder wrote:
> In dgap_parsefile() char pointers are set with kstrdup()
> without checking that some string is allocated to that
> char pointer before.

Why would this happen?  Wouldn't it be better to reject the invalid
config file?

regards,
dan carpenter

Re: [PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[Ronit Halder]

The existing dgap_parsefile() rejects invalid config file.
But before we know that config file is invalid a lot of memory leak
can happen. Removing the chances of memory leak won't heart anyone.

regards,
Ronit Halder

Re: [PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[Dan Carpenter]

On Thu, Oct 01, 2015 at 09:11:31AM +0530, Ronit Halder wrote:
> The existing dgap_parsefile() rejects invalid config file.
> But before we know that config file is invalid a lot of memory leak
> can happen. Removing the chances of memory leak won't heart anyone.
> 

Why not just reject it earlier instead of allocating more data?

regards,
dan carpenter

Re: [PATCH] staging: dgap: fix memory leak in dgap_parsefile()

[Ronit Halder]

> Why not just reject it earlier instead of allocating more data?
I agree on your point.
But we have to make different mechanism for that and my patch is only
to fix the current code.

regards