fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

There's an obvious memory leak in net/ipv4/tcp_probe.c::tcpprobe_read() We are not freeing 'tbuf' on error.
Patch below fixes that.

---

 net/ipv4/tcp_probe.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletion(-)

@@ -129,8 +129,10 @@ static ssize_t tcpprobe_read(struct file
 
 	error = wait_event_interruptible(tcpw.wait,
 					 __kfifo_len(tcpw.fifo) != 0);
-	if (error)
+	if (error) {
+		vfree(tbuf);
 		return error;
+	}
 
 	cnt = kfifo_get(tcpw.fifo, tbuf, len);
 	error = copy_to_user(buf, tbuf, cnt);

Agreed, thanks for catching it. The whole kfifo interface is kind of annoying have to do an extra copy.

Might be cleaner to make a single return path for cleanup:

---

@@ -130,11 +130,12 @@ static ssize_t tcpprobe_read(struct file
 	error = wait_event_interruptible(tcpw.wait,
 					 __kfifo_len(tcpw.fifo) != 0);
 	if (error)
-		return error;
+		goto out_free;
 
 	cnt = kfifo_get(tcpw.fifo, tbuf, len);
 	error = copy_to_user(buf, tbuf, cnt);
 
+out_free:
 	vfree(tbuf);
 
 	return error ? error : cnt;

Looks ok to me.

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

From: "Jesper Juhl" <[email protected]>
Date: Sun, 30 Jul 2006 23:51:20 +0200

I've applied James's version of the fix, thanks everyone.

Hmm, if you are refering to commit 118075b3cdc90e0815362365f3fc64d672ace0d6 -
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=118075b3cdc90e0815362365f3fc64d672ace0d6
then I think a mistake has crept in. That commit only initializes 'cnt' to 0 - I don't see how that would fix the leak - looks like you forgot the business end of the patch...

The function still looks like this after that commit :

114 static ssize_t tcpprobe_read(struct file *file, char __user *buf, 115 size_t len, loff_t *ppos)
116 {
117 int error = 0, cnt = 0;
118 unsigned char *tbuf;
119
120 if (!buf || len < 0)
121 return -EINVAL;
122
123 if (len == 0)
124 return 0;
125
126 tbuf = vmalloc(len);
127 if (!tbuf)
128 return -ENOMEM;
129
130 error = wait_event_interruptible(tcpw.wait,
131 __kfifo_len(tcpw.fifo) != 0);
132 if (error)
133 return error;
134

^^^--- we are still leaking 'tbuf' here.

135 cnt = kfifo_get(tcpw.fifo, tbuf, len);
136 error = copy_to_user(buf, tbuf, cnt);
137
138 vfree(tbuf);
139
140 return error ? error : cnt;
141 }

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

From: "Jesper Juhl" <[email protected]>
Date: Sat, 5 Aug 2006 01:30:49 +0200

See the commit right before that, the initialize of cnt to zero is just to fix a compiler warning that resulted from James's version of the fix.

Hmm, perhaps I'm going blind, but I don't see it.

The commit right before the one I linked to above is completely unrelated : "[ATALK]: Make CONFIG_DEV_APPLETALK a tristate."
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9cac2c35e26cc44978df654306bb92d7cfe7e2de

And if I download 2.6.18-rc4 the tcpprobe_read() function (still) looks like this :

static ssize_t tcpprobe_read(struct file *file, char __user *buf,
size_t len, loff_t *ppos)

{
       int error = 0, cnt = 0;
       unsigned char *tbuf;

       if (!buf || len < 0)
               return -EINVAL;

       if (len == 0)
               return 0;

       tbuf = vmalloc(len);
       if (!tbuf)
               return -ENOMEM;

       error = wait_event_interruptible(tcpw.wait,
                                        __kfifo_len(tcpw.fifo) != 0);
       if (error)
               return error;

       cnt = kfifo_get(tcpw.fifo, tbuf, len);
       error = copy_to_user(buf, tbuf, cnt);

       vfree(tbuf);

       return error ? error : cnt;
}

That function still contains the 'tbuf' leak.

I also couldn't find the fix in your git trees at
http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.19.git;a=summary
http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.git;a=summary

So either I'm going blind or a mistake has been made getting the fix into mainline...

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

From: "Jesper Juhl" <[email protected]>
Date: Fri, 11 Aug 2006 00:18:44 +0200

I definitely screwed that changeset up somehow. Thanks for catching this.

Somehow James's fix got clobbered into just my subsequent warning fix, like this:

commit 118075b3cdc90e0815362365f3fc64d672ace0d6

Author: James Morris <[email protected]>
Date:   Sun Jul 30 20:21:45 2006 -0700

[TCP]: fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

Based upon a patch by Jesper Juhl.

index d7d517a..b343532 100644
@@ -114,7 +114,7 @@ static int tcpprobe_open(struct inode * 
 static ssize_t tcpprobe_read(struct file *file, char __user *buf,
 			     size_t len, loff_t *ppos)
 {
-	int error = 0, cnt;
+	int error = 0, cnt = 0;
 	unsigned char *tbuf;
 
 	if (!buf || len < 0)

Anyways, I'll fix this up, thanks again.

Dave, here is my version...
Don't leak memory on interrupted read. And only allocate as much memory as needed.

@@ -114,7 +114,7 @@
 static ssize_t tcpprobe_read(struct file *file, char __user *buf,
 			     size_t len, loff_t *ppos)
 {
-	int error = 0, cnt = 0;
+	int error, cnt;
 	unsigned char *tbuf;
 
 	if (!buf || len < 0)

@@ -123,15 +123,16 @@
 	if (len == 0)
 		return 0;
 
-	tbuf = vmalloc(len);
-	if (!tbuf)
-		return -ENOMEM;
-
 	error = wait_event_interruptible(tcpw.wait,
 					 __kfifo_len(tcpw.fifo) != 0);
 	if (error)
 		return error;
 
+	len = min(len, kfifo_len(tcpw.fifo));
+	tbuf = vmalloc(len);
+	if (!tbuf)
+		return -ENOMEM;
+
 	cnt = kfifo_get(tcpw.fifo, tbuf, len);
 	error = copy_to_user(buf, tbuf, cnt);
 

From: Stephen Hemminger <[email protected]>
Date: Thu, 10 Aug 2006 16:52:16 -0700

I think I'm going to go with James's safe original fix for now, thanks.

commit a7fc5b24a4921a6582ce47c0faf3a31858a80468

Author: David S. Miller <[email protected]>
Date:   Thu Aug 10 16:53:33 2006 -0700

[TCP]: Fix botched memory leak fix to tcpprobe_read().

Somehow I clobbered James's original fix and only my subsequent compiler warning change went in for that changeset.

Get the real fix in there.

Noticed by Jesper Juhl.

index b343532..dab37d2 100644
@@ -130,11 +130,12 @@ static ssize_t tcpprobe_read(struct file
 	error = wait_event_interruptible(tcpw.wait,
 					 __kfifo_len(tcpw.fifo) != 0);
 	if (error)
-		return error;
+		goto out_free;
 
 	cnt = kfifo_get(tcpw.fifo, tbuf, len);
 	error = copy_to_user(buf, tbuf, cnt);
 
+out_free:
 	vfree(tbuf);
 
 	return error ? error : cnt;