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;
