[PATCH] [msan] intercept dlopen and clear shadow for it

[Reid Kleckner]

  There's still a question about the ragged edges of PT_LOAD commands.


================
Comment at: lib/msan/msan_linux.cc:125
@@ +124,3 @@
+  // Compute the delta from the real base to get a relocation delta.
+  uptr delta = (uptr)base - preferred_base;
+  // Now we can figure out what the loader really mapped.
----------------
Evgeniy Stepanov wrote:
> CHECK that base >= preferred_base.
I actually intended for this to work if base >= preferred_base, so maybe I should make delta signed.

I haven't tested this case, though.  I think you need to use pre-linking or something to make it happen.

================
Comment at: lib/msan/msan_linux.cc:134
@@ +133,3 @@
+      // mapping as defined.
+      seg_start = RoundDownTo(seg_start, GetPageSizeCached());
+      seg_end = RoundUpTo(seg_end, GetPageSizeCached());
----------------
Evgeniy Stepanov wrote:
> Why is it needed? Could we just unpoison to the exact mapping limits?
It shouldn't be needed, but I didn't like leaving stale shadow around.  The ragged edges are in fact well-defined (trailing bytes of .debug or .text before .data or trailing zeros past EOF), if not very useful.

================
Comment at: lib/msandr/msandr.cc:599
@@ +598,3 @@
+  // app segment base, which it has.  Alternatively, if we disable
+  // -mangle_app_seg and we won't need the swap.
+  bool need_swap = !dr_using_app_state(drcontext);
----------------
Evgeniy Stepanov wrote:
> s/and/,/ ?
Done, thanks.

================
Comment at: lib/msandr/msandr.cc:650
@@ +649,3 @@
+        dr_printf("unpoisoning for dlopen: [%p-%p]\n", base, base + size);
+      __msan_unpoison(base, size);
+    }
----------------
Evgeniy Stepanov wrote:
> Do we need to switch to app context here?
> We use a plain memset for the same purpose above. Use either one or the other in both places, kill MEM_TO_SHADOW if it's not needed after that.
We only need the swap if the routine ever accesses TLS.  __msan_in_loader() needs this badly, but __msan_unpoison() probably doesn't and won't for some time.

================
Comment at: lib/msan/msan_linux.cc:34
@@ +33,3 @@
+// llvm/Support/Compiler.h defines away our interface.
+#undef __msan_unpoison
+
----------------
Evgeniy Stepanov wrote:
> This is fragile. Future additions to Compiler.h can silently disable parts of MSan.
> Please move UnpoisonMappedDSO to a new .cc file, so that this include does not affect anything else.
Yeah, sounds good.  Moved to msan_elf.cc.  It is kind of worrying that the ELF type stuff that I use here might at some point become more than just a header dependence.  Do you think it would be better to include llvm/Support/ELF.h and use Elf64/32_[EP]hdr directly?


http://llvm-reviews.chandlerc.com/D509