Message ID | 20120607232652.B1CD420462@glenhelen.mtv.corp.google.com |
---|---|
State | Superseded |
Headers | show |
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.
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
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
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 --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;