diff mbox

[PATCHv2] Fix a couple memory leaks

Message ID 20120607232652.B1CD420462@glenhelen.mtv.corp.google.com
State Superseded
Headers show

Commit Message

Paul Stewart June 7, 2012, 2 a.m. UTC 
Found using valgrind.

Signed-hostap: Paul Stewart <pstew@chromium.org>
---
 src/drivers/driver_nl80211.c            |    5 +++--
 wpa_supplicant/dbus/dbus_new_handlers.c |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Jouni Malinen June 8, 2012, 5:23 p.m. UTC | #1 
On Wed, Jun 06, 2012 at 07:00:33PM -0700, Paul Stewart wrote:
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> @@ -8397,18 +8397,19 @@ static int nl80211_signal_monitor(void *priv, int threshold, int hysteresis)
>  	NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_THOLD, threshold);

Please note that NLA_PUT_U32 can jump to nla_put_failure.

>  	NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_HYST, hysteresis);
>  	nla_put_nested(msg, NL80211_ATTR_CQM, cqm);
>  
> +	nlmsg_free(cqm);

This looks fine, but..

>  	if (send_and_recv_msgs(drv, msg, NULL, NULL) == 0)
>  		return 0;
>  	msg = NULL;
>  
>  nla_put_failure:
> -	nlmsg_free(cqm);

this does not.. There is at least a theoretical possibility of
NLA_PUT_U32 using goto to skip that nlmsg_free(cqm) above.
Paul Stewart June 8, 2012, 5:31 p.m. UTC | #2 
On Fri, Jun 8, 2012 at 10:23 AM, Jouni Malinen <j@w1.fi> wrote:
> On Wed, Jun 06, 2012 at 07:00:33PM -0700, Paul Stewart wrote:
>> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
>> @@ -8397,18 +8397,19 @@ static int nl80211_signal_monitor(void *priv, int threshold, int hysteresis)
>>       NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_THOLD, threshold);
>
> Please note that NLA_PUT_U32 can jump to nla_put_failure.
>
>>       NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_HYST, hysteresis);
>>       nla_put_nested(msg, NL80211_ATTR_CQM, cqm);
>>
>> +     nlmsg_free(cqm);
>
> This looks fine, but..
>
>>       if (send_and_recv_msgs(drv, msg, NULL, NULL) == 0)
>>               return 0;
>>       msg = NULL;
>>
>>  nla_put_failure:
>> -     nlmsg_free(cqm);
>
> this does not.. There is at least a theoretical possibility of
> NLA_PUT_U32 using goto to skip that nlmsg_free(cqm) above.

Wow!  That's an eye opener.  Will send a new patch.


>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
Johannes Berg June 8, 2012, 5:33 p.m. UTC | #3 
On Fri, 2012-06-08 at 10:31 -0700, Paul Stewart wrote:
> On Fri, Jun 8, 2012 at 10:23 AM, Jouni Malinen <j@w1.fi> wrote:
> > On Wed, Jun 06, 2012 at 07:00:33PM -0700, Paul Stewart wrote:
> >> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> >> @@ -8397,18 +8397,19 @@ static int nl80211_signal_monitor(void *priv, int threshold, int hysteresis)
> >>       NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_THOLD, threshold);
> >
> > Please note that NLA_PUT_U32 can jump to nla_put_failure.
> >
> >>       NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_HYST, hysteresis);
> >>       nla_put_nested(msg, NL80211_ATTR_CQM, cqm);
> >>
> >> +     nlmsg_free(cqm);
> >
> > This looks fine, but..
> >
> >>       if (send_and_recv_msgs(drv, msg, NULL, NULL) == 0)
> >>               return 0;
> >>       msg = NULL;
> >>
> >>  nla_put_failure:
> >> -     nlmsg_free(cqm);
> >
> > this does not.. There is at least a theoretical possibility of
> > NLA_PUT_U32 using goto to skip that nlmsg_free(cqm) above.
> 
> Wow!  That's an eye opener.  Will send a new patch.

Might be worthwhile to use nla_nest_start/end instead of nla_put_nested?

johannes
Jouni Malinen June 8, 2012, 5:38 p.m. UTC | #4 
On Fri, Jun 08, 2012 at 10:31:00AM -0700, Paul Stewart wrote:
> On Fri, Jun 8, 2012 at 10:23 AM, Jouni Malinen <j@w1.fi> wrote:
> > this does not.. There is at least a theoretical possibility of
> > NLA_PUT_U32 using goto to skip that nlmsg_free(cqm) above.
> 
> Wow!  That's an eye opener.  Will send a new patch.

Yeah.. Not really the part I like about libnl, but well, that's the
design it uses. I've been thinking of converting driver_nl80211.c to
use nla_put() directly, but have not really found enough justification
and time to do so. Then again, all the current uses of NLA_PUT* should
really be analyzed anyway since this types of issues show up every now
and then. I think I once went through the full file, but well, I still
don't trust on it being 100% correct.
diff mbox

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 92a7de0..67bff35 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -8397,18 +8397,19 @@  static int nl80211_signal_monitor(void *priv, int threshold, int hysteresis)
 
 	cqm = nlmsg_alloc();
 	if (cqm == NULL)
-		return -1;
+		goto nla_put_failure;
 
 	NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_THOLD, threshold);
 	NLA_PUT_U32(cqm, NL80211_ATTR_CQM_RSSI_HYST, hysteresis);
 	nla_put_nested(msg, NL80211_ATTR_CQM, cqm);
 
+	nlmsg_free(cqm);
+
 	if (send_and_recv_msgs(drv, msg, NULL, NULL) == 0)
 		return 0;
 	msg = NULL;
 
 nla_put_failure:
-	nlmsg_free(cqm);
 	nlmsg_free(msg);
 	return -1;
 }
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
index 3a5bcab..4cab426 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers.c
@@ -614,6 +614,7 @@  DBusMessage * wpas_dbus_handler_create_interface(DBusMessage *message,
 out:
 	os_free(driver);
 	os_free(ifname);
+	os_free(confname);
 	os_free(bridge_ifname);
 	return reply;