hyrise/hyrise-v1

ValueId with uninitialized members

mrks opened this issue · 4 comments

mrks commented

I experienced an issue in an unrelated area (adding an stx btree in the DeltaIndex caused JSONTests.parse_selection to fail even though it uses the SimpleTableScan) and believe that I have tracked it down to memory corruption / an uninitialized member:

==17235== Conditional jump or move depends on uninitialised value(s)
==17235==    at 0x485CD4: ValueId::operator==(ValueId&) (storage_types.h:166)
==17235==    by 0x4A1F94: hyrise::access::EqualsExpression<long>::operator()(unsigned long) (pred_EqualsExpression.h:51)

The code in question is this:

  bool operator==(ValueId &o) {
    return valueId == o.valueId && table == o.table;
  }

In fact, there is a ValueId constructor (ValueId()) that does not set valueId or table. That constructor is used quite frequently. Is there any reason for doing so?

If I initialize valueId and table to 0 in that constructor, my bug disappears; if I initialize it to -1, I get even more failing tests (7 of 382).

grundprinzip commented

Can you create a core file of when the test fails? or break at this point in time to see member values of valueID? It would be interesting to see the callchain and identify who does not initialize the members...

bastih commented

Default-Initializing table to 0 sounds like a valid fix. On a different note, in the long-term, I'd love to move away from ValueId.table.

grundprinzip commented

Still it would be nice to understand where the call comes from...

mrks commented 
> valgrind --tool=memcheck --track-origins=yes ./build/units-access_debug
[...]
==22006==  Uninitialised value was created by a heap allocation
==22006==    at 0x4C2B1C7: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22006==    by 0x5B81BB: hyrise::access::SimpleFieldExpression* hyrise::access::expression_factory::operator()<long>() (pred_expression_factory.h:87)
==22006==    by 0x5B8066: hyrise::access::expression_factory::value_type hyrise::storage::type_switch<boost::mpl::vector<long, float, std::string, long, float, std::string, long, float, std::string, int, float, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, 0, false>::operator()<hyrise::access::expression_factory>(unsigned long, hyrise::access::expression_factory&) (meta_storage.h:44)
==22006==    by 0x5B7571: hyrise::access::buildFieldExpression(hyrise::access::PredicateType::type, Json::Value const&) (pred_buildExpression.cpp:23)
==22006==    by 0x5B776C: hyrise::access::buildExpression(Json::Value const&) (pred_buildExpression.cpp:52)
==22006==    by 0x677F94: hyrise::access::SimpleTableScan::parse(Json::Value const&) (SimpleTableScan.cpp:83)
==22006==    by 0x678BAA: hyrise::access::QueryParserFactory<hyrise::access::SimpleTableScan, hyrise::access::parse_construct>::parse(Json::Value const&) (QueryParser.h:50)
==22006==    by 0x702AAF: hyrise::access::QueryParser::parse(std::string, Json::Value const&) (QueryParser.cpp:124)
==22006==    by 0x464EDD: hyrise::access::JSONTests_parse_selection_Test::TestBody() (jsontests.cpp:274)
==22006==    by 0x8D56E7: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest-all.cpp:3393)
==22006==    by 0x8D18D7: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest-all.cpp:3429)
==22006==    by 0x8C01CC: testing::Test::Run() (gtest-all.cpp:3465)