Re: [CHECKER] 32 Memory Leaks on Error Paths

On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:

[FILE:  2.6.0-test5/net/ipv4/igmp.c]
[FUNC:  igmpv3_newpack]
[LINES: 274-284]
[VAR:   skb]
 269:	struct sk_buff *skb;
 270:	struct rtable *rt;
 271:	struct iphdr *pip;
 272:	struct igmpv3_report *pig;
 273:
START -->
 274:	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
 275:	if (skb == NULL)
 276:		return 0;
 277:
 278:	{
 279:		struct flowi fl = { .oif = dev->ifindex,
 280:				    .nl_u = { .ip4_u = {
 281:				    .daddr = IGMPV3_ALL_MCR } },
 282:				    .proto = IPPROTO_IGMP };
 283:		if (ip_route_output_key(&rt, &fl))
END -->
 284:			return 0;
 285:	}
 286:	if (rt->rt_src == 0) {
 287:		ip_rt_put(rt);
 288:		return 0;
 289:	}

Looks valid.  And since skb isn't really needed until after these
returns, moving four lines down a bit fixes the problem.

Davem, is this correct?

Jörn

-- 
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

--- linux-2.6.0-test3/net/ipv4/igmp.c~igmp_memleak	2003-07-15 23:09:02.000000000 +0200
+++ linux-2.6.0-test3/net/ipv4/igmp.c	2003-09-16 10:29:47.000000000 +0200

@@ -70,6 +70,8 @@
  *		Alexey Kuznetsov:	Accordance to igmp-v2-06 draft.
  *		David L Stevens:	IGMPv3 support, with help from
  *					Vinay Kulkarni
+ *		Jörn Engel:		Fix memleak in igmpv3_newpack, reported
+ *					by David Yu Chen (stanford checker)
  */
 
 

@@ -271,10 +273,6 @@
 	struct iphdr *pip;
 	struct igmpv3_report *pig;
 
-	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
-	if (skb == NULL)
-		return 0;
-
 	{
 		struct flowi fl = { .oif = dev->ifindex,
 				    .nl_u = { .ip4_u = {

@@ -288,6 +286,10 @@
 		return 0;
 	}
 
+	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
+	if (skb == NULL)
+		return 0;
+
 	skb->dst = &rt->u.dst;
 	skb->dev = dev;
 

On Tue, 16 Sep 2003 11:07:48 +0200
Jörn Engel [off-list ref] wrote:

Looks valid.  And since skb isn't really needed until after these
returns, moving four lines down a bit fixes the problem.

Davem, is this correct?

Nope, now you're leaking the route instead of the SKB :-)

I'll put the correct fix in.