Instrumented malloc and free with dummy functions.

src/IceASanInstrumentation.cpp

 //===- subzero/src/IceASanInstrumentation.cpp - ASan ------------*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Implements the AddressSanitizer instrumentation class.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "IceASanInstrumentation.h"
 
 #include "IceBuildDefs.h"
 #include "IceCfgNode.h"
 #include "IceGlobalInits.h"
 #include "IceInst.h"
 #include "IceTargetLowering.h"
 #include "IceTypes.h"
 
 #include <sstream>
+#include <unordered_map>
 
 namespace Ice {
 
 namespace {
 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
 // types of the redzones and their associated globals match so that they are
 // laid out together in memory.
 void ASanInstrumentation::instrumentGlobals(VariableDeclarationList &Globals) {
   if (DidInsertRedZones)
     return;
 
   VariableDeclarationList NewGlobals;
@@ skipping 63 matching lines @@
     Rz->addInitializer(
         VariableDeclaration::ZeroInitializer::create(List, RzSize));
   }
   Rz->setIsConstant(Global->getIsConstant());
   RzArray->addInitializer(VariableDeclaration::RelocInitializer::create(
       List, Rz, RelocOffsetArray(0)));
   ++RzArraySize;
   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()));
 }
 
 // TODO(tlively): Take size of access into account as well
 void ASanInstrumentation::instrumentAccess(LoweringContext &Context,
                                            Operand *Op, SizeT Size) {
   Constant *AccessCheck =
       Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_check"));
   constexpr SizeT NumArgs = 2;
@@ skipping 14 matching lines @@
   Constant *ShadowMemInit =
       Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_init"));
   constexpr SizeT NumArgs = 0;
   constexpr Variable *Void = nullptr;
   constexpr bool NoTailCall = false;
   auto *Call = InstCall::create(Func, NumArgs, Void, ShadowMemInit, NoTailCall);
   Func->getEntryNode()->getInsts().push_front(Call);
 }
 
 } // end of namespace Ice