Date: Sun, 18 Jan 2004 18:08:02 +0200 From: Ruslan Ermilov <ru@freebsd.org> To: Paul Twohey <twohey@CS.Stanford.EDU> Cc: scsi@freebsd.org Subject: Re: [CHECKER] bugs in FreeBSD Message-ID: <20040118160802.GC32115@FreeBSD.org.ua> In-Reply-To: <20040118154447.GA32115@FreeBSD.org.ua> References: <Pine.LNX.4.44.0401161607260.26554-100000@Xenon.Stanford.EDU> <20040118154447.GA32115@FreeBSD.org.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--SO98HVl1bnMOfKZd
Content-Type: multipart/mixed; boundary="yLVHuoLXiP9kZBkt"
Content-Disposition: inline
--yLVHuoLXiP9kZBkt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, Jan 18, 2004 at 05:44:48PM +0200, Ruslan Ermilov wrote:
> On Fri, Jan 16, 2004 at 04:09:34PM -0800, Paul Twohey wrote:
> [...]
> > ---------------------------------------------------------
> > [BUG]
> > /u2/engler/mc/freebsd/sys/i386/compile/GENERIC/../../../dev/dpt/dpt_scs=
i.c:1542:dpt_attach:ERROR:LEAK:1542:1571: pointer=3Ddevq from RO=3Dcam_simq=
_alloc(-1) [s=3D21,pop=3D21,pr=3D0.99] [rank=3Dmed] leaked! [z=3D1.0] [succ=
ess=3D3]
> >=20
> > int i;
> >=20
> > /*
> > * Create the device queue for our SIM.
> > */
> > Start --->
> > devq =3D cam_simq_alloc(dpt->max_dccbs);
> >=20
> > ... DELETED 23 lines ...
> >=20
> >=20
> > }
> > if (i > 0)
> > EVENTHANDLER_REGISTER(shutdown_final, dptshutdown,
> > dpt, SHUTDOWN_PRI_DEFAULT);
> > Error --->
> > return (i);
> > }
> >=20
> > int
> > ---------------------------------------------------------
>=20
> We aren't leaking "devq" here, it's freed (if necessary) by setting
> the second cam_sim_free() argument to true:
>=20
> if (xpt_bus_register(dpt->sims[i], i) !=3D CAM_SUCCESS) {
> cam_sim_free(dpt->sims[i], /*free_devq*/i =3D=3D =
0);
> break;
> }
>=20
> But we're missing the proper NULL checking, here's the fix:
>=20
> %%%
> Index: dpt_scsi.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /home/ncvs/src/sys/dev/dpt/dpt_scsi.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 dpt_scsi.c
> --- dpt_scsi.c 24 Aug 2003 17:46:04 -0000 1.45
> +++ dpt_scsi.c 18 Jan 2004 15:39:13 -0000
> @@ -1553,6 +1553,8 @@ dpt_attach(dpt_softc_t *dpt)
> dpt->sims[i] =3D cam_sim_alloc(dpt_action, dpt_poll, "dpt",
> dpt, dpt->unit, /*untagged*/2,
> /*tagged*/dpt->max_dccbs, devq);
> + if (dpt->sims[i] =3D=3D NULL)
> + break;
> if (xpt_bus_register(dpt->sims[i], i) !=3D CAM_SUCCESS) {
> cam_sim_free(dpt->sims[i], /*free_devq*/i =3D=3D 0);
> break;
> %%%
>=20
Bah, but with this patch that avoids the NULL pointer dereference
we start leaking devq. Attached is a more complete patch, and for
dev/irr/irr.c too.
Cheers,
--=20
Ruslan Ermilov
FreeBSD committer
ru@FreeBSD.org
--yLVHuoLXiP9kZBkt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=p
Index: dpt/dpt_scsi.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/dpt/dpt_scsi.c,v
retrieving revision 1.45
diff -u -p -r1.45 dpt_scsi.c
--- dpt/dpt_scsi.c 24 Aug 2003 17:46:04 -0000 1.45
+++ dpt/dpt_scsi.c 18 Jan 2004 15:51:44 -0000
@@ -1553,6 +1553,11 @@ dpt_attach(dpt_softc_t *dpt)
dpt->sims[i] = cam_sim_alloc(dpt_action, dpt_poll, "dpt",
dpt, dpt->unit, /*untagged*/2,
/*tagged*/dpt->max_dccbs, devq);
+ if (dpt->sims[i] == NULL) {
+ if (i == 0)
+ cam_simq_free(devq);
+ break;
+ }
if (xpt_bus_register(dpt->sims[i], i) != CAM_SUCCESS) {
cam_sim_free(dpt->sims[i], /*free_devq*/i == 0);
break;
Index: iir/iir.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/iir/iir.c,v
retrieving revision 1.9
diff -u -p -r1.9 iir.c
--- iir/iir.c 26 Sep 2003 15:36:47 -0000 1.9
+++ iir/iir.c 18 Jan 2004 15:52:04 -0000
@@ -510,6 +510,11 @@ iir_attach(struct gdt_softc *gdt)
gdt->sims[i] = cam_sim_alloc(iir_action, iir_poll, "iir",
gdt, gdt->sc_hanum, /*untagged*/2,
/*tagged*/GDT_MAXCMDS, devq);
+ if (gdt->sims[i] == NULL) {
+ if (i == 0)
+ cam_simq_free(devq);
+ break;
+ }
if (xpt_bus_register(gdt->sims[i], i) != CAM_SUCCESS) {
cam_sim_free(gdt->sims[i], /*free_devq*/i == 0);
break;
--yLVHuoLXiP9kZBkt--
--SO98HVl1bnMOfKZd
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (FreeBSD)
iD8DBQFACq9iUkv4P6juNwoRAghWAKCBpqGJmtW1g7vOJS15YgKfg/782QCeImr/
aZ5eUYh2kvOaSBl5zcFd4mE=
=j+I+
-----END PGP SIGNATURE-----
--SO98HVl1bnMOfKZd--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040118160802.GC32115>