Instrumented malloc and free with dummy functions.

Instrumented malloc and free with dummy functions.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=26c43064eccd7b238a97adf8c1e824cc4f2db4ea

[tlively, 2016-06-17 04:09:41]

tlively@google.com changed reviewers:
+ kschimpf@google.com, mcgrathr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2016-06-17 15:43:06]

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:37: const string_map FuncSubstitutions =
{{"malloc", "__asan_malloc"},
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.

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:124: InstCall *Inst) {
Name the arg Instr, not Inst, since Inst is the name of a type and this could
lead to compile errors down the road.

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:125: if (Inst->getCallTarget()->getKind() !=
Operand::kConstRelocatable)
I would do something like this:

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

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:131: if (FuncSubstitutions.find(TargetName) ==
FuncSubstitutions.end())
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.

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

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:37: const string_map FuncSubstitutions =
{{"malloc", "__asan_malloc"},
On 2016/06/17 15:43:05, stichnot wrote:
> 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.

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]

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:37: const string_map FuncSubstitutions =
{{"malloc", "__asan_malloc"},
On 2016/06/17 16:06:02, Karl wrote:
> On 2016/06/17 15:43:05, stichnot wrote:
> > 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.
> 
> 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.

Done.

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:124: InstCall *Inst) {
On 2016/06/17 15:43:05, stichnot wrote:
> Name the arg Instr, not Inst, since Inst is the name of a type and this could
> lead to compile errors down the road.

Done.

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:125: if (Inst->getCallTarget()->getKind() !=
Operand::kConstRelocatable)
On 2016/06/17 15:43:05, stichnot wrote:
> I would do something like this:
> 
> auto *CallTarget =
>     llvm::dyn_cast<ConstantRelocatable>(Inst->getCallTarget());
> if (CallTarget == nullptr)
>   return;

Done.

https://codereview.chromium.org/2079723002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:131: if (FuncSubstitutions.find(TargetName) ==
FuncSubstitutions.end())
On 2016/06/17 15:43:05, stichnot wrote:
> 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.

Done.

[Karl, 2016-06-17 22:04:11]

LGTM after nit is fixed.

https://codereview.chromium.org/2079723002/diff/40001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2079723002/diff/40001/src/IceInstrumentation....
src/IceInstrumentation.cpp:48: if (FuncName == "_start")
Move declaration of FuncName here.

[Jim Stichnoth, 2016-06-17 22:39:24]

lgtm

https://codereview.chromium.org/2079723002/diff/40001/docs/ASAN.rst
File docs/ASAN.rst (right):

https://codereview.chromium.org/2079723002/diff/40001/docs/ASAN.rst#newcode30
docs/ASAN.rst:30: 
Remove trailing "whitespace" (i.e. newlines) to keep git happy.

[tlively, 2016-06-17 22:53:27]

Description was changed from

==========
Instrumented malloc and free with dummy functions.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Instrumented malloc and free with dummy functions.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[tlively, 2016-06-17 22:53:29]

Committed patchset #3 (id:40001) manually as
26c43064eccd7b238a97adf8c1e824cc4f2db4ea (presubmit successful).

[tlively, 2016-06-17 23:03:59]

https://codereview.chromium.org/2079723002/diff/40001/docs/ASAN.rst
File docs/ASAN.rst (right):

https://codereview.chromium.org/2079723002/diff/40001/docs/ASAN.rst#newcode30
docs/ASAN.rst:30: 
On 2016/06/17 22:39:24, stichnot wrote:
> Remove trailing "whitespace" (i.e. newlines) to keep git happy.
I didn't see this comment until after I landed, but it will be in my next CL.

https://codereview.chromium.org/2079723002/diff/40001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2079723002/diff/40001/src/IceInstrumentation....
src/IceInstrumentation.cpp:48: if (FuncName == "_start")
On 2016/06/17 22:04:11, Karl wrote:
> Move declaration of FuncName here.

Done.