Bug 373990

Summary: Potential shift left overflow in guest_arm_toIR.c
Product: [Developer tools] valgrind Reporter: Ivo Raisr <ivosh>
Component: vexAssignee: Julian Seward <jseward>
Status: REPORTED ---    
Severity: minor CC: ivosh
Priority: NOR    
Version: 3.13 SVN   
Target Milestone: ---   
Platform: Compiled Sources   
OS: All   
Latest Commit: Version Fixed In:
Attachments: full analysis

Description Ivo Raisr 2016-12-21 08:29:50 UTC 
Created attachment 102917 [details]
full analysis

STACK (static checker for unstable code in C/C++ programs) [1] found a potential problem in several places of VEX/priv/guest_arm_toIR.c.

Here are the relevant excerpts from the analysis represented in YAML:
---
bug: anti-simplify
model: |
  %cmp2994 = icmp sge i32 %sub2976, 1, !dbg !6922
  -->  true
stack: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15239:0
ncore: 1
core: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15237:0
    - shift left overflow
---
bug: anti-simplify
model: |
  %cmp3033 = icmp sge i32 %sub2976, 1, !dbg !6960
  -->  true
stack: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15260:0
ncore: 1
core: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15237:0
    - shift left overflow
---
bug: anti-simplify
model: |
  %cmp3071 = icmp sge i32 %sub2976, 1, !dbg !7022
  -->  true
stack: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15278:0
ncore: 1
core: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15237:0
    - shift left overflow
---
bug: anti-simplify
model: |
  %cmp3118 = icmp sge i32 %sub2976, 1, !dbg !7105
  -->  true
stack: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15300:0
ncore: 1
core: 
  - /var/tmp/valgrind/VEX/priv/guest_arm_toIR.c:15237:0
    - shift left overflow
---

Full analysis output is attached.


[1] http://css.csail.mit.edu/stack/
Comment 1 Julian Seward 2017-03-06 16:17:59 UTC 
It's complaining about this

assign(scale, unop(Iop_I32UtoF64, mkU32( ((UInt)1) << (frac_bits-1) )));

in the case "VCVT fixed<->floating, VFP" (cond 1110 1D11 1p1U Vd 101f x1i0 imm4)

From a quick check of the code, it appears that:

imm4                can be 0 .. 15
((imm4 << 1) | bI)  can be 0 .. 31
size can be 16 or 32
size - ((imm4 << 1) | bI) can be 32 .. 1 (when size = 32) 
                              or 16 .. -15 (when size = 16)

and frac_bits = size - ((imm4 << 1) | bI)

So the complaint seems correct.  The subsequent checks

      if (frac_bits >= 1 && frac_bits <= 32 && !to_fixed && !dp_op
                                            && size == 32) {
 
make it safe, but yes .. it's not good.  It would be better to
have the frac_bits range check guarding the assignment to |scale|.