On Fri, 30 Nov 2018 16:55:42 +0100 Alexandre Ratchov <a...@caoua.org> wrote:
> On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote: > > Hi > > > > There is a leak of *arg in > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > > since Rev. 1.49 > > Because athn_usb_do_async() memcpy's the argument anyway. > > > > Found with llvm/scan-build. > > > > Instead of adding free(arg) I opted to make this function > > more like the other ones which call athn_usb_do_async. > > > > Hi, > > AFAICS, athn_usb_do_async() will schedule a call to > athn_usb_newauth_cb(), which will use arg after the functin has > returned. The arg memory location must stay valid after return from > athn_usb_newauth(). So we can neither use free() nor a local variable. athn_usb_do_async() takes care of that by memcpy-ing arg to cmd->data before calling usb_add_task(). other calls to athn_usb_do_async() do use local variables. if_athn_usb.c:1032:athn_usb_do_async(usc, athn_usb_newstate_cb, &cmd, sizeof(cmd)); if_athn_usb.c:1317:athn_usb_do_async(usc, athn_usb_ampdu_tx_start_cb, &cmd, sizeof(cmd)); if_athn_usb.c:1641:athn_usb_do_async(usc, athn_usb_set_key_cb, &cmd, sizeof(cmd)); if_athn_usb.c:1673:athn_usb_do_async(usc, athn_usb_delete_key_cb, &cmd, sizeof(cmd)); > The athn_usb_newauth_cb() callback calls free(), so there's no memory > leak unless the callback is cancelled. I don't know it can be > cancelled, I see no code doing so. > > > Only compile tested... looking for tests. > > > > Greetings Ben > > > > Index: if_athn_usb.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v > > retrieving revision 1.51 > > diff -u -p -r1.51 if_athn_usb.c > > --- if_athn_usb.c 6 Sep 2018 11:50:54 -0000 1.51 > > +++ if_athn_usb.c 29 Nov 2018 18:33:40 -0000 > > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic > > struct ifnet *ifp = &ic->ic_if; > > struct athn_node *an = (struct athn_node *)ni; > > int nsta; > > - struct athn_usb_newauth_cb_arg *arg; > > + struct athn_usb_newauth_cb_arg arg; > > > > if (ic->ic_opmode != IEEE80211_M_HOSTAP) > > return 0; > > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic > > * In a process context, try to add this node to the > > * firmware table and confirm the AUTH request. > > */ > > - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); > > - if (arg == NULL) > > - return ENOMEM; > > - arg->ni = ieee80211_ref_node(ni); > > - arg->seq = seq; > > - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); > > + arg.ni = ieee80211_ref_node(ni); > > + arg.seq = seq; > > + athn_usb_do_async(usc, athn_usb_newauth_cb, &arg, sizeof(arg)); > > return EBUSY; > > #else > > return 0; > > >
