[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity
On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote: > From: Bob Liu <bob.liu@xxxxxxxxxx> > > CID 1198729, CID 1198730 and CID 1198734 complain about > "Out-of-bounds read". > > This patch fixes them by casting the 'firstbyte' to (uint8_t). > > Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> > --- > xen/common/tmem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > index f2dc26e..506c6be 100644 > --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor > *pgp, struct tmem_pool > { > struct tmem_page_content_descriptor *pcd = pgp->pcd; > struct page_info *pfp = pgp->pcd->pfp; > - uint16_t firstbyte = pgp->firstbyte; > + uint8_t firstbyte = pgp->firstbyte; Actually looking at these CIDs, I think this is a coverity bug rather than a tmem bug. The two asserts ASSERT(firstbyte != NOT_SHAREABLE); /* for NOT_SHAREABLE being (uint16_t)-1UL; */ ASSERT(firstbyte < 256); Cause the coverity analysis engine to decide: "cond_const: Checking firstbyte != 65535 implies that firstbyte and pgp->firstbyte have the value 65535 on the false branch." despite the fact that the second assert entirely covering the first. Furthermore, I don't understand why the ASSERT() killpath isn't invalidating any analysis on the false branch of an ASSERT(). If you are changing uint16_t to uint8_t, you can drop those two asserts as well, as they become unconditionally true. > char *pcd_tze = pgp->pcd->tze; > pagesize_t pcd_size = pcd->size; > pagesize_t pgp_size = pgp->size; > @@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct > tmem_page_descriptor *pgp, bool_t *ho > struct tmem_object_root *obj = pgp->us.obj; > struct tmem_pool *pool = obj->pool; > struct client *client = pool->client; > - uint16_t firstbyte = pgp->firstbyte; > + uint8_t firstbyte = pgp->firstbyte; > > if ( pool->is_dying ) > return 0; Given the "if ( firstbyte == NOT_SHAREABLE ) goto obj_unlock;", are you certain this change is safe? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |