Re: memory leaks in bwfm

[Patrick Wildt]

The code hands off the reponsibility of ctl and ctl->buf to bs_txctl
which will free both buffers if there is an error enqueueing the
command.  Only if bs_txctl succeeds in enqueueing and there is a
response timeout we can free it.  Thus, not ok.  If this pattern
is not understandable then we can work on that, but the diff as is
will add double frees on error.

On Tue, Sep 18, 2018 at 03:52:45PM +1000, Jonathan Gray wrote:
> Index: bwfm.c
> ===
> RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 bwfm.c
> --- bwfm.c25 Jul 2018 20:37:11 -  1.54
> +++ bwfm.c18 Sep 2018 05:21:30 -
> @@ -1297,6 +1297,7 @@ bwfm_proto_bcdc_query_dcmd(struct bwfm_s
>  
>   if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
>   DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
>   return ret;
>   }
>  
> @@ -1337,6 +1338,7 @@ bwfm_proto_bcdc_set_dcmd(struct bwfm_sof
>  
>   if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
>   DPRINTF(("%s: txctl failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
>   return ret;
>   }
>  
> @@ -1361,6 +1363,7 @@ bwfm_proto_bcdc_txctl(struct bwfm_softc 
>  
>   if (sc->sc_bus_ops->bs_txctl(sc, ctl)) {
>   DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(ctl, M_TEMP, sizeof(*ctl));
>   return 1;
>   }
>  
> 

Re: memory leaks in bwfm

[Claudio Jeker]

On Tue, Sep 18, 2018 at 03:52:45PM +1000, Jonathan Gray wrote:
> Index: bwfm.c
> ===
> RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 bwfm.c
> --- bwfm.c25 Jul 2018 20:37:11 -  1.54
> +++ bwfm.c18 Sep 2018 05:21:30 -
> @@ -1297,6 +1297,7 @@ bwfm_proto_bcdc_query_dcmd(struct bwfm_s
>  
>   if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
>   DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
>   return ret;
>   }
>  
> @@ -1337,6 +1338,7 @@ bwfm_proto_bcdc_set_dcmd(struct bwfm_sof
>  
>   if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
>   DPRINTF(("%s: txctl failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
>   return ret;
>   }
>  
> @@ -1361,6 +1363,7 @@ bwfm_proto_bcdc_txctl(struct bwfm_softc 
>  
>   if (sc->sc_bus_ops->bs_txctl(sc, ctl)) {
>   DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(ctl, M_TEMP, sizeof(*ctl));
>   return 1;
>   }
>  
> 

There is now the possibility of a double free because of the timeout code
in bwfm_proto_bcdc_txctl(). That code does:
if (timeout) {
TAILQ_REMOVE(>sc_bcdc_rxctlq, ctl, next);
DPRINTF(("%s: timeout waiting for txctl response\n",
DEVNAME(sc)));
free(ctl->buf, M_TEMP, ctl->len);
free(ctl, M_TEMP, sizeof(*ctl));
return 1;
}

Since ctl->buf is the dcmd from e.g. bwfm_proto_bcdc_set_dcmd this results
in a double free of dcmd. I would suggest to remove the
free(ctl->buf, M_TEMP, ctl->len);
from that timeout block and then the diff is OK.

-- 
:wq Claudio

memory leaks in bwfm

[Jonathan Gray]

Index: bwfm.c
===
RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
retrieving revision 1.54
diff -u -p -r1.54 bwfm.c
--- bwfm.c  25 Jul 2018 20:37:11 -  1.54
+++ bwfm.c  18 Sep 2018 05:21:30 -
@@ -1297,6 +1297,7 @@ bwfm_proto_bcdc_query_dcmd(struct bwfm_s
 
if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
+   free(dcmd, M_TEMP, size);
return ret;
}
 
@@ -1337,6 +1338,7 @@ bwfm_proto_bcdc_set_dcmd(struct bwfm_sof
 
if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, )) {
DPRINTF(("%s: txctl failed\n", DEVNAME(sc)));
+   free(dcmd, M_TEMP, size);
return ret;
}
 
@@ -1361,6 +1363,7 @@ bwfm_proto_bcdc_txctl(struct bwfm_softc 
 
if (sc->sc_bus_ops->bs_txctl(sc, ctl)) {
DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
+   free(ctl, M_TEMP, sizeof(*ctl));
return 1;
}