[PATCH] [msan] MSanDR: a simple DynamoRio-based tool that handles uninstrumented libraries and dynamically generated code

[Reid Kleckner]

  Do you think we need more feedback from the community about if this is an OK place for this?

  I also think we should add a script or README to msandr in the initial commit to at least document how to run msandr, even if it doesn't work for everyone.


================
Comment at: lib/msandr/msandr.cc:28
@@ +27,3 @@
+
+using std::string;
+
----------------
Evgeniy Stepanov wrote:
> Kostya Serebryany wrote:
> > Can we avoid using STL?
> > It may give us lots of trouble with DR's native exec. 
> The real question here is why does it work at all? :)
> Probably because we never have both instrumented libc++/libstdc++ and run under MSanDR at the same time. With native_exec _and_ a statically linked client and DR we are probably fine, too, because all of them will live in high addresses. With native_exec and dynamic linking we might be in trouble.
For regular (no -native_exec) dynamic DR, this Just Works because we spent all that effort on our private loader.  :)

For dynamic DR with -native_exec (not ready yet anyway), I don't think we'll be able to use the private loader and the segment swapping it does.  Any STL or libc call can then clobber errno, which is a big problem.

With STATIC_LIBRARY, I have no idea why it works.  I think you'll end up using the app's STL copy which will allocate on the app heap, probably using the msan allocator, which is bad for isolation.

So, since it works with dynamic DR right now, and apparently it works somewhat with static DR, we can probably commit as-is, but we should exorcise STL stuff eventually.

================
Comment at: lib/msandr/msandr.cc:207
@@ +206,3 @@
+  };
+  std::set<reg_id_t> unused_registers(GPR_TO_USE_FOR_R2, GPR_TO_USE_FOR_R2 + 4);
+  unused_registers.erase(R1);
----------------
Evgeniy Stepanov wrote:
> Kostya Serebryany wrote:
> > grrr. std::set is not that easy to replace with non-STL. 
> > What's the maximal number of elements here? 
> 4 :)
> easily replaced with an array or even a bitfield.
Yep.

================
Comment at: lib/msandr/msandr.cc:318
@@ +317,3 @@
+  fake_mod_data.start_ = pc;
+  std::vector<ModuleData>::iterator it =
+      lower_bound(g_module_list.begin(), g_module_list.end(), fake_mod_data,
----------------
If you use_DynamoRIO_extension(drcontainers) there's a C-style resizable array called dr_vector that uses DR's heap (as well as a C-style void* hashtable, but we don't need that).  DR's heap is in the lower 2 GB, so when we instrument the STL lower_bound may explode.

DR has a vmvector_t data structure we've been meaning to share for some time, but it's a little too special cased.

================
Comment at: lib/msandr/msandr.cc:265
@@ +264,3 @@
+
+  // FIXME: I hope this does not change the flags.
+  bool res = dr_insert_get_seg_base(drcontext, bb, i, DR_SEG_FS, DR_REG_XAX);
----------------
Can just say "clobbers nothing except xax" or something.

================
Comment at: lib/msandr/msandr.cc:283
@@ +282,3 @@
+
+  // FIXME: I hope this does not change the flags.
+  bool res = dr_insert_get_seg_base(drcontext, bb, i, DR_SEG_FS, DR_REG_XAX);
----------------
ditto, no clobber

================
Comment at: lib/msandr/msandr.cc:416
@@ +415,3 @@
+    if (opcode == OP_call_ind || opcode == OP_call_far_ind ||
+        opcode == OP_jmp_ind || opcode == OP_jmp_far_ind) {
+      InstrumentIndirectBranch(drcontext, bb, i);
----------------
Hrm, tail calls through the PLT?  This should probably have a comment about why we include these.

================
Comment at: lib/msandr/msandr.cc:59
@@ +58,3 @@
+  // Full path to the module.
+  string path_;
+  module_handle_t handle_;
----------------
Kostya Serebryany wrote:
> fixed sized array, please. No STL
FWIW DR exports #define MAX_PATH 260, but it's a total lie on both Windows and Linux of course.  XD  I think we have dr_strdup() which uses DR's heap, if you want to be robust.

================
Comment at: lib/msandr/msandr.cc:288
@@ +287,3 @@
+  // TODO: unpoison more bytes?
+  PRE(i, mov_st(drcontext, OPND_CREATE_MEM64(DR_REG_XAX, msan_param_tls_offset),
+                OPND_CREATE_INT32(0)));
----------------
I guess msan is not 32-bit ready?  I still like for loop + OPND_CREATE_MEMPTR + i * sizeof(void*) better.

================
Comment at: lib/msandr/CMakeLists.txt:3
@@ +2,3 @@
+if(DynamoRIO_DIR AND DrMemoryFramework_DIR)
+  set(CMAKE_COMPILER_IS_GNUCC 1)
+  find_package(DynamoRIO)
----------------
I filed http://code.google.com/p/dynamorio/issues/detail?id=1083 on that


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