Code review comment for lp:~zorba-coders/zorba/bug907872

The ExtFuncArgItemSequence cannot live longer than the ExtFunctionCallIterator who created it, because it is just a wrapper over a child of the ExtFunctionCallIterator, and if the ExtFunctionCallIterator is destroyed, its children are destroyed as well.

The real problem is that the ExtFuncArgItemSequence is assigned to an ItemSequence_t (the return value of the evaluate() method). Since ItemSequence_t is a smart pointer, it will destroy the ExtFuncArgItemSequence when itself is destroyed. Later, when the ExtFunctionCallIterator is closed, it will also try to destroy the ExtFuncArgItemSequence.

Your fix solves this problem, so I have approved it.

> Did you also try the example (without my fix) and valgrind? I don't get a
> crash either but valgrind shows that there are serious problems:
>
> ==12383== Invalid read of size 4
> ==12383== at 0x448BDFF: zorba::SmartPtr<zorba::ItemSequence>::~SmartPtr()
> (smart_ptr.h:82)
> ==12383== by 0x4D25F43:
> zorba::ExtFunctionCallIteratorState::~ExtFunctionCallIteratorState()
> (fncall_iterator.cpp:625)
> ==12383== by 0x4D2FE87: zorba::StateTraitsImpl<zorba::ExtFunctionCallIterat
> orState>::destroyState(zorba::PlanState&, unsigned int) (plan_iterator.h:283)
> ==12383== by 0x4D2FE4D:
> zorba::NaryBaseIterator<zorba::ExtFunctionCallIterator,
> zorba::ExtFunctionCallIteratorState>::closeImpl(zorba::PlanState&)
> (narybase.h:164)
> ==12383== by 0x4D2F815:
> zorba::Batcher<zorba::ExtFunctionCallIterator>::close(zorba::PlanState&)
> (plan_iterator.h:563)
> ==12383== by 0x4CAFC27: zorba::PlanWrapper::close() (plan_wrapper.cpp:179)
> ==12383== by 0x4457EFB: zorba::XQueryImpl::execute(std::ostream&,
> Zorba_SerializerOptions const*) (xqueryimpl.cpp:1124)
> ==12383== by 0x4459B13: zorba::operator<<(std::ostream&,
> zorba::SmartPtr<zorba::XQuery> const&) (xqueryimpl.cpp:1580)
> ==12383== by 0x8068B83: external_function_test_4(zorba::Zorba*)
> (external_function.cpp:361)
> ==12383== by 0x8068E6B: external_function(int, char**)
> (external_function.cpp:407)
> ==12383== by 0x80613C4: main (UnitTests.cpp:270)
> ==12383== Address 0x7e00e28 is 0 bytes inside a block of size 24 free'd
> ==12383== at 0x4025907: operator delete(void*) (vg_replace_malloc.c:387)
> ==12383== by 0x4D2F3F5:
> zorba::ExtFuncArgItemSequence::~ExtFuncArgItemSequence()
> (fncall_iterator.cpp:539)
> ==12383== by 0x4D25F16:
> zorba::ExtFunctionCallIteratorState::~ExtFunctionCallIteratorState()
> (fncall_iterator.cpp:633)
> ==12383== by 0x4D2FE87: zorba::StateTraitsImpl<zorba::ExtFunctionCallIterat
> orState>::destroyState(zorba::PlanState&, unsigned int) (plan_iterator.h:283)
> ==12383== by 0x4D2FE4D:
> zorba::NaryBaseIterator<zorba::ExtFunctionCallIterator,
> zorba::ExtFunctionCallIteratorState>::closeImpl(zorba::PlanState&)
> (narybase.h:164)
> ==12383== by 0x4D2F815:
> zorba::Batcher<zorba::ExtFunctionCallIterator>::close(zorba::PlanState&)
> (plan_iterator.h:563)
> ==12383== by 0x4CAFC27: zorba::PlanWrapper::close() (plan_wrapper.cpp:179)
> ==12383== by 0x4457EFB: zorba::XQueryImpl::execute(std::ostream&,
> Zorba_SerializerOptions const*) (xqueryimpl.cpp:1124)
> ==12383== by 0x4459B13: zorba::operator<<(std::ostream&,
> zorba::SmartPtr<zorba::XQuery> const&) (xqueryimpl.cpp:1580)
> ==12383== by 0x8068B83: external_function_test_4(zorba::Zorba*)
> (external_function.cpp:361)
> ==12383== by 0x8068E6B: external_function(int, char**)
> (external_function.cpp:407)
> ==12383== by 0x80613C4: main (UnitTests.cpp:270)