[PATCH] drivers/usb/gadget/inode.c: add missing kfree

[PATCH] drivers/usb/gadget/inode.c: add missing kfree

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The label fail frees dev->buf, but kbuf hasn't yet been stored there at
this point.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@exists@
local idexpression x;
statement S;
expression E;
expression *ptr != NULL;
@@

x = memdup_user(...);
...
if (IS_ERR(x)) S
... when != x
    when != x = E
(
 return \(0\|<+...x...+>\|ptr\);
|
*return ...;
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
I wonder if it is really correct to free dev->buf at fail in this case.
Because it is freeing the previously allocated value of dev->buf, not the
current one as done in subsequent cases.

 drivers/usb/gadget/inode.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index a01383f..316547a 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1870,8 +1870,10 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 
 	spin_lock_irq (&dev->lock);
 	value = -EINVAL;
-	if (dev->buf)
+	if (dev->buf) {
+		kfree(kbuf);
 		goto fail;
+	}
 	dev->buf = kbuf;
 
 	/* full or low speed config */

Re: [PATCH] drivers/usb/gadget/inode.c: add missing kfree

[Alan Stern]

On Thu, 2 Jun 2011, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> The label fail frees dev->buf, but kbuf hasn't yet been stored there at
> this point.

...

> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
> I wonder if it is really correct to free dev->buf at fail in this case.
> Because it is freeing the previously allocated value of dev->buf, not the
> current one as done in subsequent cases.

I don't think it matters.  This check is probably there only for 
safety; it should not be possible for dev->buf to be non-NULL at this 
point.  You ought to be able to remove the entire

	if (dev->buf)
		goto fail;

statement with no ill effects.

Alan Stern

Re: [PATCH] drivers/usb/gadget/inode.c: add missing kfree

[Julia Lawall]

On Thu, 2 Jun 2011, Alan Stern wrote:

> On Thu, 2 Jun 2011, Julia Lawall wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > The label fail frees dev->buf, but kbuf hasn't yet been stored there at
> > this point.
> 
> ...
> 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> > I wonder if it is really correct to free dev->buf at fail in this case.
> > Because it is freeing the previously allocated value of dev->buf, not the
> > current one as done in subsequent cases.
> 
> I don't think it matters.  This check is probably there only for 
> safety; it should not be possible for dev->buf to be non-NULL at this 
> point.  You ought to be able to remove the entire
> 
> 	if (dev->buf)
> 		goto fail;
> 
> statement with no ill effects.

I can send such a patch if you think it is OK.  But I have no way to test 
it.

julia

Re: [PATCH] drivers/usb/gadget/inode.c: add missing kfree

[Alan Stern]

On Thu, 2 Jun 2011, Julia Lawall wrote:

> > I don't think it matters.  This check is probably there only for 
> > safety; it should not be possible for dev->buf to be non-NULL at this 
> > point.  You ought to be able to remove the entire
> > 
> > 	if (dev->buf)
> > 		goto fail;
> > 
> > statement with no ill effects.
> 
> I can send such a patch if you think it is OK.  But I have no way to test 
> it.

I don't either, since as far as I know the "if" condition can never be 
true.  You may be able to verify this just by reading the code.  Note 
that dev_config is used only as a field in dev_init_operations, which 
is used only in the gadgetfs_fill_super routine and therefore only when 
a file in this filesystem is opened.

Note also that the only time dev->buf becomes non-NULL is when
dev_config() succeeds, at which point fd->f_op is switched from
dev_init_operations to ep0_io_operations.  Therefore dev_config()
cannot be called successfully more than once for the same open file.

Alan Stern