ok mvs@
> On 20 Aug 2020, at 17:12, Klemens Nanni <k...@openbsd.org> wrote:
>
> On Thu, Aug 20, 2020 at 03:33:17PM +0200, Klemens Nanni 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?
> Sorry for the noise, Peter J. Philipp pointed out how I missed the +1
> byte to account for the NUL character which strlen(9) does not count.
> All corresponding malloc calls do strlen() + 1, so without it there's an
> off-by-one in the size.
>
> In my setup I do not exercise the service and concentrator name paths,
> but playing with `ifconfig pppoe0 [[-]pppoeac foo] [[-]pppoesvc bar]'
> doesn't blow up on any of those diff versions, either.
>
> Fixed diff.
>
> 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 20 Aug 2020 13:50:07 -0000
> @@ -257,15 +267,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) + 1);
> 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) + 1);
> 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 +559,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 +570,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 +624,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 +847,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) + 1);
> sc->sc_concentrator_name = NULL;
>
> len = strlen(parms->ac_name);
> @@ -830,7 +861,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) + 1);
> sc->sc_service_name = NULL;
>
> len = strlen(parms->service_name);
> @@ -1175,12 +1207,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;
