Merge lp:~zorba-coders/zorba/bug907872 into lp:zorba

[Matthias Brantner]

fix for bug #907872 (segfault when returning an input ItemSequence from an external function)

fix for bug #907872 (segfault when returning an input ItemSequence from an external function). ExtFunctionCallIterator doesn't have the exclusive ownership over the ItemSequences such that their lifecycle can exceed the lifetime of the iterator itself.

[Matthias Brantner (matthias-brantner)]

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug907872-2011-12-22T19-18-00.125Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug907872-2011-12-22T19-18-00.125Z is finished. The final status was:

All tests succeeded!

[Zorba Build Bot (zorba-buildbot)]

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1. Got: 1 Approve, 2 Pending.

[Markos Zaharioudakis (markos-za)]

Matthias, I cannot see how the example you added illustrates the case where an ExtFuncArgItemSequence must live longer than the ExtFunctionCallIterator who created it. In fact, I tried the example without your modification in fncall_iterator.cpp and it worked fine. What am I missing?

[Matthias Brantner (matthias-brantner)]

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::ExtFunctionCallIteratorState>::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::ExtFunctionCallIteratorState>::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)

[Markos Zaharioudakis (markos-za)]

review: Approve

[Markos Zaharioudakis (markos-za)]

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 Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug907872-2012-01-03T13-15-02.06Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug907872-2012-01-03T13-15-02.06Z is finished. The final status was:

All tests succeeded!

=== modified file 'ChangeLog'
--- ChangeLog	2011-12-22 14:14:53 +0000
+++ ChangeLog	2011-12-22 19:16:24 +0000
@@ -10,7 +10,8 @@
   * Added index management function to the C++ api's StaticCollectionManager.
   * Fixed bug #905041 (allow for the default element and function namespaces to be
     set multiple times via the c++ api).
-  * Fixed bug #905050 (setting and getting the context item type via the c++ api)
+  * Fixed bug #907872 (segfault when returning an input ItemSequence from an external function).
+  * Fixed bug #905050 (setting and getting the context item type via the c++ api).
   * Added createDayTimeDuration, createYearMonthDuration, createDocumentNode, createCommentNode, createPiNode to api's ItemFactory.
 
 version 2.1

=== modified file 'src/runtime/core/fncall_iterator.cpp'
--- src/runtime/core/fncall_iterator.cpp	2011-12-21 14:40:33 +0000
+++ src/runtime/core/fncall_iterator.cpp	2011-12-22 19:16:24 +0000
@@ -630,7 +630,7 @@
 
   for (csize i = 0; i < n; ++i)
   {
-    delete m_extArgs[i];
+    m_extArgs[i]->removeReference();
   }
 }
 
@@ -737,6 +737,8 @@
   for(ulong i = 0; i < n; ++i)
   {
     state->m_extArgs[i] = new ExtFuncArgItemSequence(theChildren[i], planState);
+    // the iterator does not have exlcusive ownership over the sequences
+    state->m_extArgs[i]->addReference();
   }
 }
 

=== modified file 'test/unit/CMakeLists.txt'
--- test/unit/CMakeLists.txt	2011-12-21 14:40:33 +0000
+++ test/unit/CMakeLists.txt	2011-12-22 19:16:24 +0000
@@ -28,8 +28,10 @@
   
 #belongs to test external_function.cpp
 CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/ext_mod.xq ${CMAKE_CURRENT_BINARY_DIR}/ext_mod.xq)
+CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/ext_mod2.xq ${CMAKE_CURRENT_BINARY_DIR}/ext_mod2.xq)
 CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/ext_main.xq ${CMAKE_CURRENT_BINARY_DIR}/ext_main.xq)
 CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/ext_main2.xq ${CMAKE_CURRENT_BINARY_DIR}/ext_main2.xq)
+CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/ext_main3.xq ${CMAKE_CURRENT_BINARY_DIR}/ext_main3.xq)
 
 #belongs to test no_folding.cpp
 CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/fold_mod1.xq ${CMAKE_CURRENT_BINARY_DIR}/fold_mod1.xq)

=== added file 'test/unit/ext_main3.xq'
--- test/unit/ext_main3.xq	1970-01-01 00:00:00 +0000
+++ test/unit/ext_main3.xq	2011-12-22 19:16:24 +0000
@@ -0,0 +1,21 @@
+(:
+ : Copyright 2006-2009 The FLWOR Foundation.
+ :
+ : Licensed under the Apache License, Version 2.0 (the "License");
+ : you may not use this file except in compliance with the License.
+ : You may obtain a copy of the License at
+ :
+ : http://www.apache.org/licenses/LICENSE-2.0
+ :
+ : Unless required by applicable law or agreed to in writing, software
+ : distributed under the License is distributed on an "AS IS" BASIS,
+ : WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ : See the License for the specific language governing permissions and
+ : limitations under the License.
+:)
+
+
+import module namespace ext = "http://www.zorba-xquery.com/m" at "file:///${CMAKE_CURRENT_BINARY_DIR}/ext_mod2.xq";
+
+
+ext:bar4(for $i in 1 to 10 return $i)

=== added file 'test/unit/ext_mod2.xq'
--- test/unit/ext_mod2.xq	1970-01-01 00:00:00 +0000
+++ test/unit/ext_mod2.xq	2011-12-22 19:16:24 +0000
@@ -0,0 +1,19 @@
+(:
+ : Copyright 2006-2009 The FLWOR Foundation.
+ :
+ : Licensed under the Apache License, Version 2.0 (the "License");
+ : you may not use this file except in compliance with the License.
+ : You may obtain a copy of the License at
+ :
+ : http://www.apache.org/licenses/LICENSE-2.0
+ :
+ : Unless required by applicable law or agreed to in writing, software
+ : distributed under the License is distributed on an "AS IS" BASIS,
+ : WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ : See the License for the specific language governing permissions and
+ : limitations under the License.
+:)
+
+module namespace ext = "http://www.zorba-xquery.com/m";
+
+declare function ext:bar4($s as item()*) as item()* external;

=== modified file 'test/unit/external_function.cpp'
--- test/unit/external_function.cpp	2011-10-10 19:29:05 +0000
+++ test/unit/external_function.cpp	2011-12-22 19:16:24 +0000
@@ -125,6 +125,18 @@
   }
 };
 
+class MySimpleExternalFunction4 : public NonContextualExternalFunction
+{
+public:
+  String getURI() const { return "http://www.zorba-xquery.com/m"; }
+
+  String getLocalName() const { return "bar4"; }
+
+  ItemSequence_t evaluate(const ExternalFunction::Arguments_t& args) const
+  {
+    return args[0];
+  }
+};
 
 class MyExternalModule : public ExternalModule
 {
@@ -132,6 +144,7 @@
   MySimpleExternalFunction           bar;
   MySimpleExternalFunction2          bar2;
   MySimpleExternalFunction3          bar3;
+  MySimpleExternalFunction4          bar4;
 
 public:
   String getURI() const { return "http://www.zorba-xquery.com/m"; }
@@ -140,8 +153,10 @@
   {
     if (aLocalname == "bar")
         return const_cast<MySimpleExternalFunction*>(&bar);
-    if (aLocalname == "bar3")
+    else if (aLocalname == "bar3")
         return const_cast<MySimpleExternalFunction3*>(&bar3);
+    else if (aLocalname == "bar4")
+        return const_cast<MySimpleExternalFunction4*>(&bar4);
     else
         return const_cast<MySimpleExternalFunction2*>(&bar2);
   }
@@ -327,6 +342,42 @@
   return true;
 }
 
+bool
+external_function_test_4(Zorba* aZorba)
+{
+  try 
+  {
+    std::ifstream lIn("ext_main3.xq");
+    assert(lIn.good());
+    std::ostringstream lOut;
+    MyExternalModule lMod;
+
+    StaticContext_t lSctx = aZorba->createStaticContext();
+    lSctx->registerModule(&lMod);
+
+    {
+      XQuery_t lQuery = aZorba->compileQuery(lIn, lSctx);
+
+      std::cout << lQuery << std::endl;
+    }
+  } 
+  catch (XQueryException& qe) 
+  {
+    std::cerr << qe << std::endl;
+    return false;
+  }
+  catch (ZorbaException& e)
+  {
+    std::cerr << e << std::endl;
+    return false;
+  }
+  catch (...)
+  {
+    return false;
+  }
+  return true;
+}
+
 
 int
 external_function(int argc, char* argv[]) 
@@ -352,6 +403,13 @@
     return 3;
   }
 
+  std::cout << "executing external_function_test_4" << std::endl;
+  if (!external_function_test_4(lZorba))
+  {
+    return 4;
+  }
+
+
   lZorba->shutdown();
   zorba::StoreManager::shutdownStore(lStore);
   return 0;