Instrumented malloc and free with dummy functions.

src/IceASanInstrumentation.cpp

Index: src/IceASanInstrumentation.cpp
diff --git a/src/IceASanInstrumentation.cpp b/src/IceASanInstrumentation.cpp
index 589bf1a04f3090c3db04824db05317a8e6a8e145..31327f1b58e2991a7b94ef52d8495b0d9b3e2b44 100644
--- a/src/IceASanInstrumentation.cpp
+++ b/src/IceASanInstrumentation.cpp
@@ -22,6 +22,7 @@
 #include "IceTypes.h"
 
 #include <sstream>
+#include <unordered_map>
 
 namespace Ice {
 
@@ -30,6 +31,12 @@ constexpr SizeT RzSize = 32;
 const std::string RzPrefix = "__$rz";
 const llvm::NaClBitcodeRecord::RecordVector RzContents =
     llvm::NaClBitcodeRecord::RecordVector(RzSize, 'R');
+
+// TODO(tlively): Handle all allocation functions
+using string_map = std::unordered_map<std::string, std::string>;
+const string_map FuncSubstitutions = {{"malloc", "__asan_malloc"},

[Jim Stichnoth, 2016/06/17 15:43:05]

I'm wondering if you've fully considered how this would actually work in
practice.

Normally, the developer runs pnacl-clang or pnacl-clang++, which produces a
statically linked pexe containing the full newlib malloc/free implementation
complete with all supporting functions.  Then the developer runs pnacl-finalize
which, as a side effect, strips all the symbol names except for _start.

So when you enable asan, it won't be possible to identify calls to malloc or
free, at least not by name.

The developer can suppress symbol stripping by running "pnacl-finalize
--no-strip-syms", and then Subzero will succeed in finding malloc/free calls. 
This should be documented, both in the code and in whatever developer
documentation is eventually provided.

I'm also concerned about calls to auxiliary functions that might affect the
malloc heap state, like how Subzero calls mallopt() in main.cpp, but I suppose
that is a general asan issue and one not unique to Subzero.

[Karl, 2016/06/17 16:06:02]

Thomas and I have discussed this issue. Because the ASAN sanitizer must change
how malloc works, but malloc has already been statically added to the pexe,
there was a problem. This forced the need to not
strip symbols, so that we can find the malloc function in the pexe.

However, I agree with Jim that what is being done should be well documented.

I also agree that these assumptions should be well documented.

I think you should be creating external documentation, like ASAN.rst (similar to
the DESIGN.ast) that clarifies what needs to be done, and what assumptions are
being made.

[tlively, 2016/06/17 21:39:40]

Done.

+                                      {"free", "__asan_free"}};
+
 } // end of anonymous namespace
 
 // Create redzones around all global variables, ensuring that the initializer
@@ -113,14 +120,37 @@ ASanInstrumentation::createRz(VariableDeclarationList *List,
   return Rz;
 }
 
+void ASanInstrumentation::instrumentCall(LoweringContext &Context,
+                                         InstCall *Inst) {

[Jim Stichnoth, 2016/06/17 15:43:05]

Name the arg Instr, not Inst, since Inst is the name of a type and this could
lead to compile errors down the road.

[tlively, 2016/06/17 21:39:40]

Done.

+  if (Inst->getCallTarget()->getKind() != Operand::kConstRelocatable)

[Jim Stichnoth, 2016/06/17 15:43:05]

I would do something like this:

auto *CallTarget =
    llvm::dyn_cast<ConstantRelocatable>(Inst->getCallTarget());
if (CallTarget == nullptr)
  return;

[tlively, 2016/06/17 21:39:40]

Done.

+    return;
+
+  ConstantRelocatable *CallTarget =
+      static_cast<ConstantRelocatable *>(Inst->getCallTarget());
+  std::string TargetName = CallTarget->getName().toStringOrEmpty();
+  if (FuncSubstitutions.find(TargetName) == FuncSubstitutions.end())

[Jim Stichnoth, 2016/06/17 15:43:05]

Would be nice to do something like

  auto Subst = FuncSubstitutions.find(TargetName);
  if (Subst == FuncSubstitutions.end())
    return;
  std::string SubName = Subst->second;

in order to call find() just once.

[tlively, 2016/06/17 21:39:40]

Done.

+    return;
+
+  std::string SubName = FuncSubstitutions.find(TargetName)->second;
+  Constant *Substitution =
+      Ctx->getConstantExternSym(Ctx->getGlobalString(SubName));
+  auto *NewCall =
+      InstCall::create(Context.getNode()->getCfg(), Inst->getNumArgs(),
+                       Inst->getDest(), Substitution, Inst->isTailcall());
+  for (SizeT I = 0, Args = Inst->getNumArgs(); I < Args; ++I)
+    NewCall->addArg(Inst->getArg(I));
+  Context.insert(NewCall);
+  Inst->setDeleted();
+}
+
 void ASanInstrumentation::instrumentLoad(LoweringContext &Context,
-                                         const InstLoad *Inst) {
+                                         InstLoad *Inst) {
   instrumentAccess(Context, Inst->getSourceAddress(),
                    typeWidthInBytes(Inst->getDest()->getType()));
 }
 
 void ASanInstrumentation::instrumentStore(LoweringContext &Context,
-                                          const InstStore *Inst) {
+                                          InstStore *Inst) {
   instrumentAccess(Context, Inst->getAddr(),
                    typeWidthInBytes(Inst->getData()->getType()));
 }