Memory leak in if_arp.c

[Maxime Villard]

Hi,
after hearing about bouyer@'s mbuf leak problem, I launched my code scanner on
netinet/, and it found a bug:

-------------------------- netinet/if_arp.c l.1476 --------------------------

if ((m = m_gethdr(M_DONTWAIT, MT_DATA)) == NULL)
return;
[...]
if (tha == NULL)
return;

-----------------------------------------------------------------------------

'm' is leaked. However, this is not my area of working; can someone fix it?

(and it doesn't solve his problem)

Regards,
Maxime

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Motoyuki OHMORI]

Hi,
(snip)

Post by Maxime Villard
'm' is leaked. However, this is not my area of working; can someone fix it?
(and it doesn't solve his problem)

How about this? It may not be directly relevant to his problem though.

*** src/sys/netinet/if_arp.c.orig Sat Apr 12 19:07:24 2014
--- src/sys/netinet/if_arp.c Sat Apr 12 19:15:01 2014
***************
*** 1473,1480 ****

memcpy(ar_sha(ah), CLLADDR(ifp->if_sadl), ah->ar_hln);
tha = ar_tha(ah);
! if (tha == NULL)
return;
memcpy(tha, CLLADDR(ifp->if_sadl), ah->ar_hln);

sa.sa_family = AF_ARP;
--- 1473,1482 ----

memcpy(ar_sha(ah), CLLADDR(ifp->if_sadl), ah->ar_hln);
tha = ar_tha(ah);
! if (tha == NULL) {
! m_freem(m);
return;
+ }
memcpy(tha, CLLADDR(ifp->if_sadl), ah->ar_hln);

sa.sa_family = AF_ARP;

Best regards,
--

Post by Maxime Villard
Hi,
-------------------------- netinet/if_arp.c l.1476 --------------------------
if ((m = m_gethdr(M_DONTWAIT, MT_DATA)) == NULL)
return;
[...]
if (tha == NULL)
return;
-----------------------------------------------------------------------------
'm' is leaked. However, this is not my area of working; can someone fix it?
(and it doesn't solve his problem)
Regards,
Maxime

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

Post by Maxime Villard
-------------------------- netinet/if_arp.c l.1476 --------------------------
if ((m = m_gethdr(M_DONTWAIT, MT_DATA)) == NULL)
return;
[...]
if (tha == NULL)
return;
-----------------------------------------------------------------------------
'm' is leaked. However, this is not my area of working; can someone fix it?

I think you're right; I've made a change to free m on the error path.
However, I think ar_tha can only fail if the protocol field is 1394, so
with the current if_arp.h implementation, this should never have leaked.

[Greg Troxel]

Post by Motoyuki OHMORI
(snip)

Post by Maxime Villard
'm' is leaked. However, this is not my area of working; can someone fix it?
(and it doesn't solve his problem)

How about this? It may not be directly relevant to his problem though.
*** src/sys/netinet/if_arp.c.orig Sat Apr 12 19:07:24 2014
--- src/sys/netinet/if_arp.c Sat Apr 12 19:15:01 2014
***************
*** 1473,1480 ****
memcpy(ar_sha(ah), CLLADDR(ifp->if_sadl), ah->ar_hln);
tha = ar_tha(ah);
! if (tha == NULL)
return;
memcpy(tha, CLLADDR(ifp->if_sadl), ah->ar_hln);
sa.sa_family = AF_ARP;
--- 1473,1482 ----
memcpy(ar_sha(ah), CLLADDR(ifp->if_sadl), ah->ar_hln);
tha = ar_tha(ah);
! if (tha == NULL) {
! m_freem(m);
return;
+ }
memcpy(tha, CLLADDR(ifp->if_sadl), ah->ar_hln);
sa.sa_family = AF_ARP;

That's byte-for-byte what I committed - KNF doesn't leave much room to
vary given the needed fix :-)

[Manuel Bouyer]

Post by Greg Troxel

Post by Maxime Villard
-------------------------- netinet/if_arp.c l.1476 --------------------------
if ((m = m_gethdr(M_DONTWAIT, MT_DATA)) == NULL)
return;
[...]
if (tha == NULL)
return;
-----------------------------------------------------------------------------
'm' is leaked. However, this is not my area of working; can someone fix it?

I think you're right; I've made a change to free m on the error path.
However, I think ar_tha can only fail if the protocol field is 1394, so
with the current if_arp.h implementation, this should never have leaked.

Yes, that's what I understood too.