[llvm-commits] [llvm] r157261 - in /llvm/trunk: include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h include/llvm/Transforms/Utils/Local.h lib/Transforms/InstCombine/InstCombine.h lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Scalar/BoundsChecking.cpp lib/Transforms/Scalar/Scalar.cpp

[Nuno Lopes]

>> add a new pass to instrument loads and stores for run-time bounds checking
>> move EmitGEPOffset from InstCombine to Transforms/Utils/Local.h
>
> if I understand right you are inserting code that does something like this:
>
> ...
>    if (pointer < object_start || pointer > object_end) trap();
>    %x = load pointer
> ...
>
> I don't think this will work reliably.  Suppose the optimizers discover that
> pointer is undef.  The undef will be propagated throughout:
>
>    if (undef < object_start || undef > object_end) trap();
>    %x = load undef
>
> The optimizers can fold the comparisons to whatever they want, for example
>
>    if (false || false) trap();
>    %x = load undef
>
> which then turns into
>
>    %x = load undef
>
> Oops - the bound check was removed but a wrong address is still loaded!
> A similar problem may occur if the pointer is a trap value, due to an
> inbounds GEP or something like that: the compares may be eliminated even
> though the value lies outside the range.

I believe a 'load undef' will be removed. So, if there is no  
load/store, then memory is not touched, and there is no buffer  
overflow (which is what we are trying to avoid).
Of course that with this approach we will miss some bugs, namely those  
that incur in undefined behavior that is exploited by the compiler.  
But as far as security is concerned, this is fine.


> A possible solution is to introduce a new readnone intrinsic "llvm.identity"
> which simply returns its argument but is opaque to the optimizers.  You then
> insert
>
>    safe_pointer = call llvm.identity(pointer)
>    if (safe_pointer < object_start || safe_pointer > object_end) trap();
>    %x = load safe_pointer
>
> If pointer turns out to be undef, then you get
>
>    safe_pointer = call llvm.identity(undef)
>    if (safe_pointer < object_start || safe_pointer > object_end) trap();
>    %x = load safe_pointer
>
> and everything remains consistent.  The code generators or the optimizers
> can turn llvm.identity(undef) into zero for example, and codegen can turn
> llvm.identity(p) into p at some fairly late point when you know that new
> undefs aren't going to be produced or propagated any more.
>
> The llvm.identity intrinsic doesn't have to be completely opaque to the
> optimizers.  If the argument is a sensible constant like the address  
> of a global
> then llvm.identity(@global) can be replaced with @global and likewise for
> similar safe situations.
>
> By having llvm.identity be readnone it should be hoisted out of loops if
> it is loop invariant, so hopefully this solution isn't too costly.

I guess the problem then is that llvm.identity can be hoisted, but not  
llvm.trap.
Imagine you have a simple loop:

for (i=1..N)
   a[i] = ..

We want to have a single check outside of the loop, instead of one per  
iteration.
If you add the trap inside the loop, then the optimizers cannot hoist  
it out of the loop, since it would change the semantics of the program  
(imagine that you're doing a printf in the loop, for example).  To fix  
this problem, we would need a new llvm.trap_hoistable intrinsic, which  
could be safely hoisted and moved upwards in the CFG, even if it  
changed the semantics of the code.
I guess for debugging purposes, I agree this could be potentially  
better. But we are trying to be fast, and that means playing nice with  
the optimizers.


Thanks for the review!
Nuno