[llvm-commits] MemorySanitizer, first patch

[Kostya Serebryany]

On Wed, Jun 27, 2012 at 7:30 PM, John Criswell <criswell at illinois.edu>wrote:

>  On 6/26/12 11:20 AM, Kostya Serebryany wrote:
>
> John,
>
> On Tue, Jun 26, 2012 at 7:45 PM, John Criswell <criswell at illinois.edu>wrote:
>
>>  On 6/26/12 4:18 AM, Kostya Serebryany wrote:
>>
>> Updated the patch to use the new interface for TLS
>> (GlobalVariable::GeneralDynamicTLSModel).
>> Comments?
>>
>>
>>  First, there's the non-technical question of whether this belongs in
>> LLVM mainline or not in its current state.  Do you have users of this tool
>> in the same way that you had users of ASan?  What platforms have you tested
>> this tool on?  Do you get false positives when you link with native code
>> libraries, and if so, is this false positive rate low?  I get the
>> impression that this tool isn't as mature as TSan or ASan,
>>
>
>  Your impression is correct -- msan is not yet usable. It works only on
> linux-x86_64, only on small libc-free tests.
> It has been heavily tested (on SPEC and other large programs), but w/o
> actually poisoning the shadow.
>
>
>> so I'm wondering whether it's really ready to go in yet.
>>
>
>  Having these 500 LOC in trunk will simplify further development a lot.
> (we will also need a few lines of driver changes).
> I can't tell whether this justifies the commit to trunk now (it certainly
> does for me).
>
>
> I think it would be better to fix the external code issue first before
> trying to commit to LLVM trunk; it's the only obstacle I see that prevents
> you from having a working tool.  That way, you can have a working tool and
> get feedback on it; you'll also know what the ramifications are for your
> approach to handling external library code.  As it is right now, I think
> this new tool is still in too early a stage to add to LLVM.
>

Having this code in trunk will simplify and speedup the development of the
dynamic component(s).
It may also inspire some research in the area of LLVM-based binary
instrumentation.
But I'll agree on what the community says.



>
>
>
>
>>
>> Second, on the technical side, I think dealing with the external code
>> problem has to be addressed before the code can be committed; I suspect the
>> tool will return too many false positives if you don't address it (if you
>> have experiments indicating otherwise, please let us know).  I'm assuming
>> from the comment at the beginning of the pass's implementation that it's
>> still unaddressed.
>>
>
>  We are working on this, in fact experimenting in a few different
> directions (DynamoRIO, PIN, static instrumentation, even full recompile).
>
>
> Okay.
>
>
>
>
>>  I think instrumenting all library code is unrealistic; you'd have to
>> have every library on your system compiled with the sanitizer (at least for
>> large GUI applications).
>>
>
>  It may still be doable in some restricted environments, but yes, I agree
> in general.
>
>
>>  I think there's a few other approaches that you can take:
>>
>> 1) Assume external code works properly.  To do this, you first ensure
>> that memory allocated by external code is always marked as initialized.
>> Second, when passing memory objects from internal, instrumented code to
>> external library code, you mark the memory as initialized (since you're
>> assuming the external code will initialize it).
>>
>
>  That doesn't work.
> What if I pass my pointer to a library function.
> What has to be un-poisoned (marked as inited)?
> The contents of the heap/stack/bss block to which this pointer belongs?
>
>
> Yes, that's what I was thinking.
>
>
>   How about pointers that reside in this block? Do we need to go deeper?
>
>
> Ah, that's a problem.  You might be able to do this conservatively by
> treating values that point into valid memory objects as pointers (like
> conservative garbage collection), but it'd be expensive and probably not
> worth the trouble.
>

It'll be prohibitively expensive, if at all possible.
Even with all the hacks we put into the allocator code, checking whether a
given integer is a currently addressable pointer is not free. For stack
buffers that's even more expensive.
Some pointers may point slightly off the buffer (e.g. to -1'th element).


>
>
>
>
>>
>> Of course, you can do things like special casing the C library to reduce
>> false negatives, and you can instruct users to compile library code with
>> the compiler to find more bugs.
>>
>> 2) Use something like SAFECode's CompleteChecks pass to determine which
>> checks are on memory objects handled completely within the analyzed portion
>> of the program and which checks can access memory handled by external
>> code.  Checks on memory objects handled by external code always pass (or
>> just generate a warning instead of an error, or something like that).  This
>> approach requires a good points-to analysis algorithm.  SAFECode uses DSA
>> which is unification-based.  You can either use that or, if you prefer, use
>> a subset based algorithm like Anderson's.  I think there's one
>> implementation of Anderson's for LLVM out there, but I don't know how
>> robust it is.
>>
>
>  I'll have to look at this more, but I don't (yet) see how it can help.
> Again, suppose
>
>  struct A { int stuff; }
> struct B { A *a; }
> struct C { A *a, B* b};
> ...
> struct Z { A* a, B *b, C *c, ... Y *y;}
>
>  ...
> Z *z = new Z;
> library_function(z);
>
>  library_function can write to any part of Z, but we can't even traverse
> all of it.
> Can we do anything?
>
>
> No, you can't really say anything about the object to which Z points or
> the objects reachable from the pointer Z.  Any checks on z, z->a, etc.
> would have to conservatively pass.
>



> This approach doesn't help you check for initialization errors with
> library code; it helps prevent library code from triggering false
> positives.  It ensures that all of your checks are performed on memory
> objects handled by internal code.
>
> For memory safety, relaxing the safety guarantees for memory objects
> passed to library code still results in a usable system that catches memory
> safety errors.  I don't know if the same is true for uninitialized value
> detection; you'll probably have to experiment and see.
>

That's not true for uninitialized value detection.

both asan and tsan know nothing about the code in the libraries and work
just fine.
They miss some percentage of bugs (based on experience with valgrind our
estimate is 5%-10% of bugs).
Adding a dynamic component to asan/tsan will add that coverage, and we are
going to do it.

For msan the story is completely different.
You either cover everything including system calls or don't go there.
Any memory write which the tool does not observe followed by a read in the
instrumented code leads to a false report.



>
>
>
>
>
>>
>> 3) Use dynamic binary instrumentation to handle external code.
>>
>> The first option is probably the easiest to implement.  The second option
>> requires a points-to analysis that can go into LLVM mainline.  The third
>> option is a fair amount of work but probably easier than using points-to
>> analysis and will probably yield a better debugging tool.
>>
>
>  We *are* working on #3 (see above).
>
>
> I think using dynamic binary translation is probably the best option out
> of the three.  Just take my opinion with the caveat that I haven't run any
> experiments on uninitialized value usage.
>

It's actually the worst option, but the only one I know how to implement.
I'd really prefer a fully static approach (static binary translation, or
full recompile) but I don't see any suitable technology. :(


>
>> Third, other technical nit-picks:
>>
>> 1) IMHO, in general, every method and function should have a comment
>> explaining what it does and the meaning of its inputs and return values.
>> Descriptions should use complete sentences.
>>
>
>  Agree, I'll add comments.
>
>
>>
>> 2) You may want to consider using a class derived from InstVisitor
>> instead of writing large if/then/else or switch code.
>>
>
>  I Didn't know about InstVisitor.
> However, I am not sure if that would be simpler. Will have a closer look.
>
>
> InstVisitor is faster than using dyn_cast<>() and is the same as using a
> switch on the instruction opcode (which you code does in some places).  It
> may make your code more readable.
>

I am convinced. The new patch uses InstVisitor.


>
>
>
>
>>
>> 3) If I understand correctly, a "shadow instruction" propagates
>> information about initialization status through a computation (so, for
>> example, if you have a c = add a, b instruction, there's a shadow
>> instruction that computes c's initialization status based on a and b's
>> initialization status).
>>
>
>  Correct.
>
>
>>  It looks like you add shadow instructions for all instructions and then
>> "fill in" the operands to the shadow instructions by iterating over the
>> instructions a second time.  Is this correct?
>>
>
>  Yes.
>
>
> How do you handle passing taint (i.e., initialization status) between
> actual parameters and formal parameters in a function call?  Do you store
> the information to memory and then read it in the callee?
>

Yes, the shadow is passed via a dedicated thread-local memory slot.


>
>
>
>
>> If so, there's an alternative approach: if you fetch the dominator tree
>> for the function and visit instructions in dominator tree order, then
>> you'll visit all definitions before all uses.  That will alleviate the need
>> to make two passes over all the instructions.  The only tricky part is phi
>> nodes, but for those, you add shadow instructions (phi nodes with undef
>> operands) to them first and then visit all the other instructions in
>> dominator tree order; you fill in the operands of the shadow phi nodes at
>> the end.
>>
>> I'm not sure which approach is faster (I don't think LLVM's dominator
>> tree pass is the most efficient implementation), but it's something to
>> think about.
>>
>
>  I actually did think about this. I did not experiment with using
> the dominator tree, but I think that the current way is at least as
> efficient.
>
>
> It's really a question of whether computing the dominator tree is faster
> or slower than iterating over all instructions twice (or whether dominator
> tree has already been computed for some previously run transform).  I
> suppose dominator tree might be better if you're not going to instrument
> all instructions.
>

Unless I am mistaken depth-first-order is enough.
The new patch uses that.


Thanks!

--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120627/7100a6c2/attachment.html>