> On 20 Aug 2020, at 16:33, Klemens Nanni <k...@openbsd.org> wrote: > > These are straight forward as we either maintain a size variable all the > way or can reuse strlen() for free() just like it's done during malloc(). > > One exception is freeing the softc structure, which is fixed in size; > `ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code > path and does not blow up - as expected. > > Running fine on a octeon vdsl2 router. > > Feedback? OK?
Hi. You forgot about ‘\0’ for `sc_concentrator_name’ and `sc_service_name'. > > > Index: if_pppoe.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppoe.c,v > retrieving revision 1.70 > diff -u -p -r1.70 if_pppoe.c > --- if_pppoe.c 28 Jul 2020 09:52:32 -0000 1.70 > +++ if_pppoe.c 19 Aug 2020 23:33:23 -0000 > @@ -257,15 +264,17 @@ pppoe_clone_destroy(struct ifnet *ifp) > if_detach(ifp); > > if (sc->sc_concentrator_name) > - free(sc->sc_concentrator_name, M_DEVBUF, 0); > + free(sc->sc_concentrator_name, M_DEVBUF, > + strlen(sc->sc_concentrator_name)); > if (sc->sc_service_name) > - free(sc->sc_service_name, M_DEVBUF, 0); > + free(sc->sc_service_name, M_DEVBUF, > + strlen(sc->sc_service_name)); > if (sc->sc_ac_cookie) > - free(sc->sc_ac_cookie, M_DEVBUF, 0); > + free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len); > if (sc->sc_relay_sid) > - free(sc->sc_relay_sid, M_DEVBUF, 0); > + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len); > > - free(sc, M_DEVBUF, 0); > + free(sc, M_DEVBUF, sizeof(*sc)); > > return (0); > } > @@ -547,7 +556,8 @@ breakbreak: > } > if (ac_cookie) { > if (sc->sc_ac_cookie) > - free(sc->sc_ac_cookie, M_DEVBUF, 0); > + free(sc->sc_ac_cookie, M_DEVBUF, > + sc->sc_ac_cookie_len); > sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF, > M_DONTWAIT); > if (sc->sc_ac_cookie == NULL) > @@ -557,7 +567,8 @@ breakbreak: > } > if (relay_sid) { > if (sc->sc_relay_sid) > - free(sc->sc_relay_sid, M_DEVBUF, 0); > + free(sc->sc_relay_sid, M_DEVBUF, > + sc->sc_relay_sid_len); > sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF, > M_DONTWAIT); > if (sc->sc_relay_sid == NULL) > @@ -610,11 +621,12 @@ breakbreak: > sc->sc_state = PPPOE_STATE_INITIAL; > memcpy(&sc->sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest)); > if (sc->sc_ac_cookie) { > - free(sc->sc_ac_cookie, M_DEVBUF, 0); > + free(sc->sc_ac_cookie, M_DEVBUF, > + sc->sc_ac_cookie_len); > sc->sc_ac_cookie = NULL; > } > if (sc->sc_relay_sid) { > - free(sc->sc_relay_sid, M_DEVBUF, 0); > + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len); > sc->sc_relay_sid = NULL; > } > sc->sc_ac_cookie_len = 0; > @@ -817,7 +835,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned > } > > if (sc->sc_concentrator_name) > - free(sc->sc_concentrator_name, M_DEVBUF, 0); > + free(sc->sc_concentrator_name, M_DEVBUF, > + strlen(sc->sc_concentrator_name)); > sc->sc_concentrator_name = NULL; > > len = strlen(parms->ac_name); > @@ -830,7 +849,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned > } > > if (sc->sc_service_name) > - free(sc->sc_service_name, M_DEVBUF, 0); > + free(sc->sc_service_name, M_DEVBUF, > + strlen(sc->sc_service_name)); > sc->sc_service_name = NULL; > > len = strlen(parms->service_name); > @@ -1175,12 +1195,12 @@ pppoe_disconnect(struct pppoe_softc *sc) > sc->sc_state = PPPOE_STATE_INITIAL; > memcpy(&sc->sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest)); > if (sc->sc_ac_cookie) { > - free(sc->sc_ac_cookie, M_DEVBUF, 0); > + free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len); > sc->sc_ac_cookie = NULL; > } > sc->sc_ac_cookie_len = 0; > if (sc->sc_relay_sid) { > - free(sc->sc_relay_sid, M_DEVBUF, 0); > + free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len); > sc->sc_relay_sid = NULL; > } > sc->sc_relay_sid_len = 0; >