On 21/04/15 12:13, Dan Carpenter wrote:
On Mon, Apr 20, 2015 at 11:49:04AM -0700, H Hartley Sweeten wrote:
'b_chans' may be a valud up to 32. 'b_mask' is an unsigned int and a left shift of
more than 31 bits has undefined behavior. Fix the calc so it works correctly with
a 'b_chans' of 32..
Reported-by: coverity (CID 1192244)
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/drivers/comedi_bond.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 96db0c2..50b76ec 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -101,7 +101,8 @@ static int bonding_dio_insn_bits(struct comedi_device *dev,
b_chans = bdev->nchans - base_chan;
if (b_chans > n_left)
b_chans = n_left;
- b_mask = (1U << b_chans) - 1;
+ b_mask = (b_chans < 32) ? ((1 << b_chans) - 1)
+ : 0xffffffff;
Is that really an improvement? The original code was actually defined.
1U << 32 is actually defined. It is zero. Which works for us.
According to the C standards it is undefined (for 32-bit unsigned int).
See the late draft of ISO/IEC 9899:2011 (dubbed C11) at
<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf>, §6.5.7,
Semantics:
3. The integer promotions are performed on each of the operands. The
type of the result is that of the promoted left operand.If the value of
the right operand is negative or is greater than or equal to the width
of the promoted left operand, the behavior is undefined.
1 << 32 is undefined. It could be anything.
1 << 31 is INT_MIN.
Technically, 1 << 31 is undefined as 1*2^32 is not representable as a
non-negative value (see item 4 of the semantics), but in practice will
be INT_MIN for a 32-bit, 2's complement, signed int.
I think INT_MIN - 1 might be undefined. But I'm not sure. Although, in
my testing "INT_MIN - 1" and "(uint)INT_MIN - 1" are the same which is
what we intended so maybe the new code is fine.
It would be better to use (1U << b_chans) instead of (1 << b_chans) in
the code above, or possibly the slightly more obtuse:
b_mask = ((b_chans < 32) ? 1 << b_chans : 0) - 1;
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Web: http://www.mev.co.uk/ )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
