Re: possible memory leak in ipmi get_sdr
On 2022/04/07 13:31, Moritz Buhl wrote:
> Any insights?
> On Mon, Jan 10, 2022 at 03:18:47PM +0100, Moritz Buhl wrote:
> > Hi tech@,
> >
> > The return value of add_child_sensors is returned in add_sdr_sensor,
> > which is called by get_sdr. get_sdr mallocs psdr and only frees it
> > if add_sdr_sensor returns 0. The assumption is that psdr is attached
> > to a list in add_child_sensors otherwise. This is not the case if
> > the malloc for psensor fails, or read_sensor() equals 0. In any
> > of these error cases psdr is leaked.
I think you mean "This is not the case if the malloc for psensor fails,
or read_sensor() _DOES NOT_ equal 0"?
Anyway, the diff makes sense and is ok sthen@
While I'm thinking of ipmi(4), any comments on
https://marc.info/?l=openbsd-tech&m=164460370813852&w=2?
(Return values in the ipmi.c code are pretty messy, mixture of rc and
rv variable names, and 1/-1 as well as errno values, and the existing
handling in read_sensor() seems a bit silly, though not functionally
bad).
> > Index: sys/dev/ipmi.c
> > ===
> > RCS file: /mount/openbsd/cvs/src/sys/dev/ipmi.c,v
> > retrieving revision 1.115
> > diff -u -p -r1.115 ipmi.c
> > --- sys/dev/ipmi.c 23 Jan 2021 12:10:08 - 1.115
> > +++ sys/dev/ipmi.c 10 Jan 2022 13:49:19 -
> > @@ -1374,7 +1374,7 @@ add_child_sensors(struct ipmi_softc *sc,
> > int sensor_num, int sensor_type, int ext_type, int sensor_base,
> > int entity, const char *name)
> > {
> > - int typ, idx;
> > + int typ, idx, rc = 0;
> > struct ipmi_sensor *psensor;
> > #ifdef IPMI_DEBUG
> > struct sdrtype1 *s1 = (struct sdrtype1 *)psdr;
> > @@ -1415,11 +1415,12 @@ add_child_sensors(struct ipmi_softc *sc,
> > dbg_printf(5, " reading: %lld [%s]\n",
> > psensor->i_sensor.value,
> > psensor->i_sensor.desc);
> > + rc = 1;
> > } else
> > free(psensor, M_DEVBUF, sizeof(*psensor));
> > }
> >
> > - return (1);
> > + return (rc);
> > }
> >
> > /* Handle IPMI Timer - reread sensor values */
> >
>
Re: possible memory leak in ipmi get_sdr
Any insights?
On Mon, Jan 10, 2022 at 03:18:47PM +0100, Moritz Buhl wrote:
> Hi tech@,
>
> The return value of add_child_sensors is returned in add_sdr_sensor,
> which is called by get_sdr. get_sdr mallocs psdr and only frees it
> if add_sdr_sensor returns 0. The assumption is that psdr is attached
> to a list in add_child_sensors otherwise. This is not the case if
> the malloc for psensor fails, or read_sensor() equals 0. In any
> of these error cases psdr is leaked.
>
> Kind regards
> mbuhl
>
> Index: sys/dev/ipmi.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/dev/ipmi.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 ipmi.c
> --- sys/dev/ipmi.c23 Jan 2021 12:10:08 - 1.115
> +++ sys/dev/ipmi.c10 Jan 2022 13:49:19 -
> @@ -1374,7 +1374,7 @@ add_child_sensors(struct ipmi_softc *sc,
> int sensor_num, int sensor_type, int ext_type, int sensor_base,
> int entity, const char *name)
> {
> - int typ, idx;
> + int typ, idx, rc = 0;
> struct ipmi_sensor *psensor;
> #ifdef IPMI_DEBUG
> struct sdrtype1 *s1 = (struct sdrtype1 *)psdr;
> @@ -1415,11 +1415,12 @@ add_child_sensors(struct ipmi_softc *sc,
> dbg_printf(5, " reading: %lld [%s]\n",
> psensor->i_sensor.value,
> psensor->i_sensor.desc);
> + rc = 1;
> } else
> free(psensor, M_DEVBUF, sizeof(*psensor));
> }
>
> - return (1);
> + return (rc);
> }
>
> /* Handle IPMI Timer - reread sensor values */
>
Re: possible memory leak in ipmi get_sdr
On Fri, 14 Jan 2022 09:25:53 +0100
Moritz Buhl wrote:
> I thought I'd walk through my understanding of the code
> to make it easier to review.
>
> from sys/dev/ipmi.c:
>
> 1019 int
> 1020 get_sdr(struct ipmi_softc *sc, u_int16_t recid, u_int16_t *nxtrec)
> 1021 {
> ...
> 1046 psdr = malloc(sdrlen, M_DEVBUF, M_NOWAIT);
> 1047 if (psdr == NULL)
> 1048 return (1);
> Now psdr is allocated. In the following loop it is freed correctly on
> failure:
> 1062 free(psdr, M_DEVBUF, sdrlen);
> 1063 return (1);
> If everything works, we attach it to a list:
> 1067 /* Add SDR to sensor list, if not wanted, free buffer */
> 1068 if (add_sdr_sensor(sc, psdr, sdrlen) == 0)
> 1069 free(psdr, M_DEVBUF, sdrlen);
> 1070
> 1071 return (0);
> So if add_sdr_sensor doesn't return 0 it has to keep a reference
> to psdr somewhere.
>
> 1335 int
> 1336 add_sdr_sensor(struct ipmi_softc *sc, u_int8_t *psdr, int sdrlen)
> 1337 {
> There are two switch cases that both call add_child_sensors and set
> rc with it, if no previous failures occour:
> 1349 rc = add_child_sensors(sc, psdr, 1, s1->sensor_num,
> 1350 s1->sensor_type, s1->event_code, 0, s1->entity_id,
> name);
> 1351 break;
> and
> 1358 rc = add_child_sensors(sc, psdr, s2->share1 & 0xF,
> 1359 s2->sensor_num, s2->sensor_type, s2->event_code,
> 1360 s2->share2 & 0x7F, s2->entity_id, name);
> 1361 break;
> These two calls are the only locations that can make add_sdr_sensor
> return something else than 0.
>
> 1370 int
> 1371 add_child_sensors(struct ipmi_softc *sc, u_int8_t *psdr, int count,
> 1372 int sensor_num, int sensor_type, int ext_type, int sensor_base,
> 1373 int entity, const char *name)
> 1374 {
> So as mentioned before, this function needs to return 0 or keep a
> reference to psdr.
> This is not the case in two situations, but to keep it simple I
> will only show one:
> 1388 psensor = malloc(sizeof(*psensor), M_DEVBUF, M_NOWAIT |
> M_ZERO);
> 1389 if (psensor == NULL)
> 1390 break;
> ...
> 1420 return (1);
>
> I hope this helps
> mbuhl
>
Hi, llvm/scan-build agrees.
http://www.netzbasis.de/openbsd/scan-build/kernel/2022-01-12-131800-47421-1/report-c7382e.html#EndPath
Patch seems correct, but I'm not a dev and I don't have the hardware eighter.
Greetings Ben
Re: possible memory leak in ipmi get_sdr
I thought I'd walk through my understanding of the code
to make it easier to review.
from sys/dev/ipmi.c:
1019 int
1020 get_sdr(struct ipmi_softc *sc, u_int16_t recid, u_int16_t *nxtrec)
1021 {
...
1046 psdr = malloc(sdrlen, M_DEVBUF, M_NOWAIT);
1047 if (psdr == NULL)
1048 return (1);
Now psdr is allocated. In the following loop it is freed correctly on
failure:
1062 free(psdr, M_DEVBUF, sdrlen);
1063 return (1);
If everything works, we attach it to a list:
1067 /* Add SDR to sensor list, if not wanted, free buffer */
1068 if (add_sdr_sensor(sc, psdr, sdrlen) == 0)
1069 free(psdr, M_DEVBUF, sdrlen);
1070
1071 return (0);
So if add_sdr_sensor doesn't return 0 it has to keep a reference
to psdr somewhere.
1335 int
1336 add_sdr_sensor(struct ipmi_softc *sc, u_int8_t *psdr, int sdrlen)
1337 {
There are two switch cases that both call add_child_sensors and set
rc with it, if no previous failures occour:
1349 rc = add_child_sensors(sc, psdr, 1, s1->sensor_num,
1350 s1->sensor_type, s1->event_code, 0, s1->entity_id,
name);
1351 break;
and
1358 rc = add_child_sensors(sc, psdr, s2->share1 & 0xF,
1359 s2->sensor_num, s2->sensor_type, s2->event_code,
1360 s2->share2 & 0x7F, s2->entity_id, name);
1361 break;
These two calls are the only locations that can make add_sdr_sensor
return something else than 0.
1370 int
1371 add_child_sensors(struct ipmi_softc *sc, u_int8_t *psdr, int count,
1372 int sensor_num, int sensor_type, int ext_type, int sensor_base,
1373 int entity, const char *name)
1374 {
So as mentioned before, this function needs to return 0 or keep a
reference to psdr.
This is not the case in two situations, but to keep it simple I
will only show one:
1388 psensor = malloc(sizeof(*psensor), M_DEVBUF, M_NOWAIT |
M_ZERO);
1389 if (psensor == NULL)
1390 break;
...
1420 return (1);
I hope this helps
mbuhl
possible memory leak in ipmi get_sdr
Hi tech@,
The return value of add_child_sensors is returned in add_sdr_sensor,
which is called by get_sdr. get_sdr mallocs psdr and only frees it
if add_sdr_sensor returns 0. The assumption is that psdr is attached
to a list in add_child_sensors otherwise. This is not the case if
the malloc for psensor fails, or read_sensor() equals 0. In any
of these error cases psdr is leaked.
Kind regards
mbuhl
Index: sys/dev/ipmi.c
===
RCS file: /mount/openbsd/cvs/src/sys/dev/ipmi.c,v
retrieving revision 1.115
diff -u -p -r1.115 ipmi.c
--- sys/dev/ipmi.c 23 Jan 2021 12:10:08 - 1.115
+++ sys/dev/ipmi.c 10 Jan 2022 13:49:19 -
@@ -1374,7 +1374,7 @@ add_child_sensors(struct ipmi_softc *sc,
int sensor_num, int sensor_type, int ext_type, int sensor_base,
int entity, const char *name)
{
- int typ, idx;
+ int typ, idx, rc = 0;
struct ipmi_sensor *psensor;
#ifdef IPMI_DEBUG
struct sdrtype1 *s1 = (struct sdrtype1 *)psdr;
@@ -1415,11 +1415,12 @@ add_child_sensors(struct ipmi_softc *sc,
dbg_printf(5, " reading: %lld [%s]\n",
psensor->i_sensor.value,
psensor->i_sensor.desc);
+ rc = 1;
} else
free(psensor, M_DEVBUF, sizeof(*psensor));
}
- return (1);
+ return (rc);
}
/* Handle IPMI Timer - reread sensor values */
