[DynInst_API:] Beware a global BPatch trying to detach

[Josh Stone <jistone@xxxxxxxxxx>]

I've had a fun time recently trying to nail down this bug.  If a mutator
has its BPatch as a global variable, attaches to a mutatee, then exits
without explicitly detaching, the mutator will segfault.

This problem is easily avoided if your BPatch object has a tighter
scope, e.g. as a local in main().  It's also fine if you explicitly
detach from mutatees before exiting.  So that's my high level advice to
offer in this mail, but read on if you like details.

The gist is that ~BPatch calls ~BPatch_process for all mutatees, which
may then call PCProcess::detachProcess.  This will attempt to remove the
instrumentation on fork/exec/exit which dyninst implicitly adds.  That's
a good thing, or else the detached mutatee will get a SIGBUS if it later
reaches one of those events.  However, this attempt to remove
instrumentation hits a SEGV deep in instructionAPI.

Valgrind memcheck can identify where bad memory was accessed.  There are
many such errors before it hits something that's actually corrupted and
crashes, but they all point back to the same allocation.  Here's the
first full message:

> ==9968== Thread 1:
> ==9968== Invalid read of size 4
> ==9968==    at 0x53A4B98: Dyninst::InstructionAPI::InstructionDecoderImpl::makeAddExpression(boost::shared_ptr<Dyninst::InstructionAPI::Expression>, boost::shared_ptr<Dyninst::InstructionAPI::Expression>, Dyninst::InstructionAPI::Result_Type) (sp_counted_base_gcc_x86.hpp:66)
> ==9968==    by 0x5355718: Dyninst::InstructionAPI::InstructionDecoder_x86::decodeOneOperand(Dyninst::InstructionAPI::InstructionDecoder::buffer const&, NS_x86::ia32_operand const&, int&, Dyninst::InstructionAPI::Instruction const*, bool, bool) (InstructionDecoder-x86.C:860)
> ==9968==    by 0x53581FC: Dyninst::InstructionAPI::InstructionDecoder_x86::decodeOperands(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder-x86.C:1224)
> ==9968==    by 0x5351939: Dyninst::InstructionAPI::InstructionDecoder_x86::doDelayedDecode(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder-x86.C:1243)
> ==9968==    by 0x534EF97: Dyninst::InstructionAPI::InstructionDecoder::doDelayedDecode(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder.C:77)
> ==9968==    by 0x5338AF4: Dyninst::InstructionAPI::Instruction::decodeOperands() const (Instruction.C:119)
> ==9968==    by 0x53390AB: Dyninst::InstructionAPI::Instruction::format(unsigned long) const (Instruction.C:445)
> ==9968==    by 0x4D44BE9: Dyninst::Relocation::CFWidget::CFWidget(boost::shared_ptr<Dyninst::InstructionAPI::Instruction>, unsigned long) (CFWidget.C:89)
> ==9968==    by 0x4D4539A: Dyninst::Relocation::CFWidget::create(boost::shared_ptr<Dyninst::Relocation::Widget>) (CFWidget.C:74)
> ==9968==    by 0x4D3ADBC: Dyninst::Relocation::RelocBlock::createCFWidget() (RelocBlock.C:328)
> ==9968==    by 0x4D3CFF1: Dyninst::Relocation::RelocBlock::createReloc(block_instance*, func_instance*) (RelocBlock.C:101)
> ==9968==    by 0x4D36D9B: Dyninst::Relocation::CodeMover::addRelocBlock(block_instance*, func_instance*) (CodeMover.C:104)
> ==9968==    by 0x4D38EC6: bool Dyninst::Relocation::CodeMover::addRelocBlocks<std::_Rb_tree_const_iterator<Dyninst::PatchAPI::PatchBlock*> >(std::_Rb_tree_const_iterator<Dyninst::PatchAPI::PatchBlock*>, std::_Rb_tree_const_iterator<Dyninst::PatchAPI::PatchBlock*>, func_instance*) (CodeMover.C:98)
> ==9968==    by 0x4D37182: Dyninst::Relocation::CodeMover::addFunctions(std::_Rb_tree_const_iterator<func_instance*>, std::_Rb_tree_const_iterator<func_instance*>) (CodeMover.C:82)
> ==9968==    by 0x4CD7B7F: AddressSpace::relocateInt(std::_Rb_tree_const_iterator<func_instance*>, std::_Rb_tree_const_iterator<func_instance*>, unsigned long) (addressSpace.C:1744)
> ==9968==    by 0x4CD8A4F: AddressSpace::relocate() (addressSpace.C:1712)
> ==9968==    by 0x4D624A2: Dyninst::PatchAPI::DynInstrumenter::run() (DynInstrumenter.C:54)
> ==9968==    by 0x5BF55D9: Dyninst::PatchAPI::Patcher::run() (Command.C:112)
> ==9968==    by 0x5BF4E71: Dyninst::PatchAPI::Command::commit() (Command.C:54)
> ==9968==    by 0x4CD376E: AddressSpace::patch(AddressSpace*) (addressSpace.C:2297)
> ==9968==    by 0x4D8B52B: syscallNotification::removePreFork() (syscallNotification.C:196)
> ==9968==    by 0x4D1C169: PCProcess::detachProcess(bool) (dynProcess.C:1090)
> ==9968==    by 0x4C8E154: BPatch_process::~BPatch_process() (BPatch_process.C:420)
> ==9968==    by 0x4C8E2C6: BPatch_process::~BPatch_process() (BPatch_process.C:456)
> ==9968==    by 0x4C736A9: BPatch::~BPatch() (BPatch.C:193)
> ==9968==    by 0x3C690394C8: __run_exit_handlers (exit.c:82)
> ==9968==    by 0x3C69039514: exit (exit.c:104)
> ==9968==    by 0x3C69021D6B: (below main) (libc-start.c:319)
> ==9968==  Address 0x12781f58 is 8 bytes inside a block of size 24 free'd
> ==9968==    at 0x4A07991: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==9968==    by 0x533F49C: boost::detail::sp_counted_impl_p<Dyninst::InstructionAPI::BinaryFunction::addResult>::~sp_counted_impl_p() (sp_counted_impl.hpp:53)
> ==9968==    by 0x533A38A: boost::detail::sp_counted_base::destroy() (sp_counted_base_gcc_x86.hpp:126)
> ==9968==    by 0x533F6E1: boost::shared_ptr<Dyninst::InstructionAPI::BinaryFunction::funcT>::~shared_ptr() (sp_counted_base_gcc_x86.hpp:160)
> ==9968==    by 0x3C690394C8: __run_exit_handlers (exit.c:82)
> ==9968==    by 0x3C69039514: exit (exit.c:104)
> ==9968==    by 0x3C69021D6B: (below main) (libc-start.c:319)
> ==9968==   block was alloc'd at
> ==9968==    at 0x4A06965: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==9968==    by 0x53A4780: Dyninst::InstructionAPI::InstructionDecoderImpl::makeAddExpression(boost::shared_ptr<Dyninst::InstructionAPI::Expression>, boost::shared_ptr<Dyninst::InstructionAPI::Expression>, Dyninst::InstructionAPI::Result_Type) (shared_count.hpp:130)
> ==9968==    by 0x5355718: Dyninst::InstructionAPI::InstructionDecoder_x86::decodeOneOperand(Dyninst::InstructionAPI::InstructionDecoder::buffer const&, NS_x86::ia32_operand const&, int&, Dyninst::InstructionAPI::Instruction const*, bool, bool) (InstructionDecoder-x86.C:860)
> ==9968==    by 0x53581FC: Dyninst::InstructionAPI::InstructionDecoder_x86::decodeOperands(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder-x86.C:1224)
> ==9968==    by 0x5351939: Dyninst::InstructionAPI::InstructionDecoder_x86::doDelayedDecode(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder-x86.C:1243)
> ==9968==    by 0x534EF97: Dyninst::InstructionAPI::InstructionDecoder::doDelayedDecode(Dyninst::InstructionAPI::Instruction const*) (InstructionDecoder.C:77)
> ==9968==    by 0x5338AF4: Dyninst::InstructionAPI::Instruction::decodeOperands() const (Instruction.C:119)
> ==9968==    by 0x5339874: Dyninst::InstructionAPI::Instruction::getControlFlowTarget() const (Instruction.C:432)
> ==9968==    by 0x5E604C1: Dyninst::InsnAdapter::IA_IAPI::getCFT() const (IA_IAPI.C:772)
> ==9968==    by 0x5E5DABD: Dyninst::InsnAdapter::IA_IAPI::isRealCall() const (IA_IAPI.C:732)
> ==9968==    by 0x5E5E080: Dyninst::InsnAdapter::IA_IAPI::hasCFT() const (IA_IAPI.C:268)
> ==9968==    by 0x5E47419: Dyninst::ParseAPI::Parser::parse_frame(Dyninst::ParseAPI::ParseFrame&, bool) (Parser.C:1237)
> ==9968==    by 0x5E482D0: Dyninst::ParseAPI::Parser::parse_frames(std::vector<Dyninst::ParseAPI::ParseFrame*, std::allocator<Dyninst::ParseAPI::ParseFrame*> >&, bool) (Parser.C:405)
> ==9968==    by 0x5E49785: Dyninst::ParseAPI::Parser::parse_vanilla() (Parser.C:278)
> ==9968==    by 0x5E49855: Dyninst::ParseAPI::Parser::parse() (Parser.C:156)
> ==9968==    by 0x5E49919: Dyninst::ParseAPI::Parser::finalize(Dyninst::ParseAPI::Function*) (Parser.C:633)
> ==9968==    by 0x5E4FE8E: Dyninst::ParseAPI::Function::finalize() (Function.C:184)
> ==9968==    by 0x5BE9560: Dyninst::PatchAPI::PatchFunction::blocks() (CFG.h:489)
> ==9968==    by 0x5BE96C5: Dyninst::PatchAPI::PatchFunction::entry() (PatchFunction.C:78)
> ==9968==    by 0x5BEF143: Dyninst::PatchAPI::PatchMgr::getFuncCandidates(Dyninst::PatchAPI::Scope&, Dyninst::PatchAPI::Point::Type, std::vector<std::pair<Dyninst::PatchAPI::Location, Dyninst::PatchAPI::Point::Type>, std::allocator<std::pair<Dyninst::PatchAPI::Location, Dyninst::PatchAPI::Point::Type> > >&) (PatchMgr.C:230)
> ==9968==    by 0x5BF11DD: Dyninst::PatchAPI::PatchMgr::getCandidates(Dyninst::PatchAPI::Scope&, Dyninst::PatchAPI::Point::Type, std::vector<std::pair<Dyninst::PatchAPI::Location, Dyninst::PatchAPI::Point::Type>, std::allocator<std::pair<Dyninst::PatchAPI::Location, Dyninst::PatchAPI::Point::Type> > >&) (PatchMgr.C:170)
> ==9968==    by 0x4D086B8: bool Dyninst::PatchAPI::PatchMgr::findPoints<Dyninst::PatchAPI::PatchMgr::IdentityFilterFunc<char*>, char*, std::back_insert_iterator<std::vector<Dyninst::PatchAPI::Point*, std::allocator<Dyninst::PatchAPI::Point*> > > >(Dyninst::PatchAPI::Scope, Dyninst::PatchAPI::Point::Type, Dyninst::PatchAPI::PatchMgr::IdentityFilterFunc<char*>, char*, std::back_insert_iterator<std::vector<Dyninst::PatchAPI::Point*, std::allocator<Dyninst::PatchAPI::Point*> > >, bool) (PatchMgr.h:249)
> ==9968==    by 0x4D08856: bool Dyninst::PatchAPI::PatchMgr::findPoints<std::back_insert_iterator<std::vector<Dyninst::PatchAPI::Point*, std::allocator<Dyninst::PatchAPI::Point*> > > >(Dyninst::PatchAPI::Scope, Dyninst::PatchAPI::Point::Type, std::back_insert_iterator<std::vector<Dyninst::PatchAPI::Point*, std::allocator<Dyninst::PatchAPI::Point*> > >, bool) (PatchMgr.h:293)
> ==9968==    by 0x4D27331: PCProcess::installInstrRequests(std::vector<instMapping*, std::allocator<instMapping*> > const&) (dynProcess.C:1878)
> ==9968==    by 0x4D8CEC8: syscallNotification::installPreFork() (syscallNotification.C:92)
> ==9968==    by 0x4D22AF3: PCProcess::bootstrapProcess() (dynProcess.C:465)
> ==9968==    by 0x4D24348: PCProcess::attachProcess(std::string const&, int, BPatch_hybridMode) (dynProcess.C:159)
> ==9968==    by 0x4C8AF71: BPatch_process::BPatch_process(char const*, int, BPatch_hybridMode) (BPatch_process.C:328)
> ==9968==    by 0x4C740E9: BPatch::processAttach(char const*, int, BPatch_hybridMode) (BPatch.C:1247)
> ==9968==    by 0x401155: main (mutator.cpp:46)

So it's a use-after-free from this allocation:

> Expression::Ptr InstructionDecoderImpl::makeAddExpression(Expression::Ptr lhs,
>         Expression::Ptr rhs, Result_Type resultType)
> {
>     static BinaryFunction::funcT::Ptr adder(new BinaryFunction::addResult());
>                                                                                                        
>     return make_shared(singleton_object_pool<BinaryFunction>::construct(lhs, rhs, resultType, adder));
> }

Static locals like "adder" are not constructed until first reached,
which in this case will be much later than our global BPatch object.
Then destructors are called in reverse order, (via __cxa_atexit), so
"adder" will be freed before our global BPatch object is destroyed.
Therefore, we can't call into anything with such a static local.

"Doc, when I move like this, it hurts!"
"Then don't move like that!"

So that gets back to my initial advice - don't use a global BPatch
object, because its destructor may access other global or static local
data.  If you must, this particular error may be avoided by detaching
from all mutatees before exiting.

It might be nicer if dyninst as a whole avoided any non-POD static
locals like this, since one can't rely on the order of their
destruction.  But that may take a large audit to track them all down,
even if we agree on this policy.


Josh