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

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

[Jesper Juhl]

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.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

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

--- linux-2.6.18-rc3-orig/net/ipv4/tcp_probe.c	2006-07-30 13:21:53.000000000 +0200
+++ linux-2.6.18-rc3/net/ipv4/tcp_probe.c	2006-07-30 21:32:04.000000000 +0200
@@ -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);

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

[Stephen Hemminger]

On Sun, 30 Jul 2006 21:38:02 +0200
Jesper Juhl <jesper.juhl@gmail.com> wrote:

> 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.
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Agreed, thanks for catching it.  The whole kfifo interface is kind of
annoying have to do an extra copy.

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

[James Morris]

On Sun, 30 Jul 2006, Stephen Hemminger wrote:

> On Sun, 30 Jul 2006 21:38:02 +0200
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
> 
> > 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.
> > 
> > 
> > Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> 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:

Signed-off-by: James Morris <jmorris@namei.org>

---


--- linux-2.6.18-rc2-mm1.o/net/ipv4/tcp_probe.c	2006-07-28 11:01:46.000000000 -0400
+++ linux-2.6.18-rc2-mm1.w/net/ipv4/tcp_probe.c	2006-07-30 17:45:53.000000000 -0400
@@ -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;

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

[Jesper Juhl]

On 30/07/06, James Morris <jmorris@namei.org> wrote:
> On Sun, 30 Jul 2006, Stephen Hemminger wrote:
>
> > On Sun, 30 Jul 2006 21:38:02 +0200
> > Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > > 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.
> > >
> > >
> > > Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> > 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:
>
> Signed-off-by: James Morris <jmorris@namei.org>
>
Looks ok to me.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
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

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

[David Miller]

From: "Jesper Juhl" <jesper.juhl@gmail.com>
Date: Sun, 30 Jul 2006 23:51:20 +0200

> Looks ok to me.

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

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

[Jesper Juhl]

On 31/07/06, David Miller <davem@davemloft.net> wrote:
> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> Date: Sun, 30 Jul 2006 23:51:20 +0200
>
> > Looks ok to me.
>
> 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 <jesper.juhl@gmail.com>
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

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

[David Miller]

From: "Jesper Juhl" <jesper.juhl@gmail.com>
Date: Sat, 5 Aug 2006 01:30:49 +0200

> On 31/07/06, David Miller <davem@davemloft.net> wrote:
> > From: "Jesper Juhl" <jesper.juhl@gmail.com>
> > Date: Sun, 30 Jul 2006 23:51:20 +0200
> >
> > > Looks ok to me.
> >
> > 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...

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.

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

[Jesper Juhl]

On 05/08/06, David Miller <davem@davemloft.net> wrote:
> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> Date: Sat, 5 Aug 2006 01:30:49 +0200
>
> > On 31/07/06, David Miller <davem@davemloft.net> wrote:
> > > From: "Jesper Juhl" <jesper.juhl@gmail.com>
> > > Date: Sun, 30 Jul 2006 23:51:20 +0200
> > >
> > > > Looks ok to me.
> > >
> > > 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...
>
> 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 <jesper.juhl@gmail.com>
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

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

[David Miller]

From: "Jesper Juhl" <jesper.juhl@gmail.com>
Date: Fri, 11 Aug 2006 00:18:44 +0200

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

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 <jmorris@namei.org>
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.
    
    Signed-off-by: James Morris <jmorris@namei.org>
    Acked-by: Stephen Hemminger <shemminger@osdl.org>
    Acked-by: Jesper Juhl <jesper.juhl@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index d7d517a..b343532 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -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.

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

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- linux-2.6.orig/net/ipv4/tcp_probe.c	2006-08-10 16:32:36.000000000 -0700
+++ linux-2.6/net/ipv4/tcp_probe.c	2006-08-10 16:45:30.000000000 -0700
@@ -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);
 

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

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Thu, 10 Aug 2006 16:52:16 -0700

> Dave, here is my version...
> Don't leak memory on interrupted read. And only allocate
> as much memory as needed.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

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

commit a7fc5b24a4921a6582ce47c0faf3a31858a80468
Author: David S. Miller <davem@sunset.davemloft.net>
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.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index b343532..dab37d2 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -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;