Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()
On Fri, Mar 31, 2017 at 05:18:39PM +0100, Stefan Hajnoczi wrote: > On Fri, Mar 31, 2017 at 10:40:33AM -0300, Eduardo Habkost wrote: > > On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote: > > > Hi, > > > > > > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to > > > write one to find those overflows? > > > > Probably not. AFAIK, Coccinelle rules are based on local code > > syntax only. This means it doesn't know the data type of > > expressions like (s->tracks). > > I'm surprised by that statement. Coccinelle isn't a text matcher, it's > a proper C compiler frontend that parses the all code in the compilation > unit. Therefore it must have the type information even for s->tracks. You are probably not wrong about it not being just a text matcher. But I'm not sure about it being able to have type information for s->tracks. The documentation isn't clear about that. The 'idexpression' declarations seems to accept some kind of C type annotations (I didn't know that!), but the documentation also says: "A more complex description of a location, such as a->b is considered to be an expression, not an idexpression". And 'expression' metavariables don't seem to support type annotations. My impression is that Coccinelle has limited support to understand simple variable declarations, but not the full set of C type declarations and type system rules that would allow it to figure out the type of an expression like s->tracks. But I really hope to be wrong, because that would be very useful. :) > Disclaimer: This should in no way be considered a volunteer offer to > write cocci scripts now or at any time in the future :). I'm not fluent > in the semantic patch syntax. I don't believe there's anybody in the world fluent in the SmPL syntax. Maybe its authors are, but I wouldn't be so sure. :) -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()
On Fri, Mar 31, 2017 at 10:40:33AM -0300, Eduardo Habkost wrote: > On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote: > > Hi, > > > > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to > > write one to find those overflows? > > Probably not. AFAIK, Coccinelle rules are based on local code > syntax only. This means it doesn't know the data type of > expressions like (s->tracks). I'm surprised by that statement. Coccinelle isn't a text matcher, it's a proper C compiler frontend that parses the all code in the compilation unit. Therefore it must have the type information even for s->tracks. Disclaimer: This should in no way be considered a volunteer offer to write cocci scripts now or at any time in the future :). I'm not fluent in the semantic patch syntax. Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()
On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote: > Hi, > > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to > write one to find those overflows? Probably not. AFAIK, Coccinelle rules are based on local code syntax only. This means it doesn't know the data type of expressions like (s->tracks). > > Peter having one more macro might help or confuses more? > > #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32) > > On 03/31/2017 10:13 AM, Peter Maydell wrote: > > Coverity (CID 1307776) points out that in the multiply: > > space = to_allocate * s->tracks; > > we are trying to calculate a 64 bit result but the types > > of to_allocate and s->tracks mean that we actually calculate > > a 32 bit result. Add an explicit cast to force a 64 bit > > multiply. > > > > Signed-off-by: Peter Maydell > > Reviewed-by: Philippe Mathieu-Daudé > > > --- > > NB: compile-and-make-check tested only... > > --- > > block/parallels.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/block/parallels.c b/block/parallels.c > > index 4173b3f..3886c30 100644 > > --- a/block/parallels.c > > +++ b/block/parallels.c > > @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, > > int64_t sector_num, > > } > > > > to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; > > -space = to_allocate * s->tracks; > > +space = (int64_t)to_allocate * s->tracks; > > if (s->data_end + space > bdrv_getlength(bs->file->bs) >> > > BDRV_SECTOR_BITS) { > > int ret; > > space += s->prealloc_size; > > -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()
On 31 March 2017 at 14:27, Philippe Mathieu-Daudé wrote: > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to > write one to find those overflows? This is the final one that Coverity reports on the current codebase. > Peter having one more macro might help or confuses more? > > #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32) We've fixed them with casts generally in the past. thanks -- PMM
Re: [Qemu-block] [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()
Hi, Eduardo you seem skilled regarding Coccinelle scripts, is it possible to write one to find those overflows? Peter having one more macro might help or confuses more? #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32) On 03/31/2017 10:13 AM, Peter Maydell wrote: Coverity (CID 1307776) points out that in the multiply: space = to_allocate * s->tracks; we are trying to calculate a 64 bit result but the types of to_allocate and s->tracks mean that we actually calculate a 32 bit result. Add an explicit cast to force a 64 bit multiply. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé --- NB: compile-and-make-check tested only... --- block/parallels.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 4173b3f..3886c30 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, } to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; -space = to_allocate * s->tracks; +space = (int64_t)to_allocate * s->tracks; if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) { int ret; space += s->prealloc_size;