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. 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; > --
