> On 20 Aug 2020, at 16:33, Klemens Nanni <[email protected]> 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;
>
