In testing the bdbstore code, I found that AccumulatedAck.range
is used uninitialized. Here's part of valgrind's report:
==3863== Thread 7:
==3863== Conditional jump or move depends on uninitialised value(s)
==3863== at 0x4B4A63F: qpid::broker::AccumulatedAck::update(unsigned long,
bool) (AccumulatedAck.cpp:29)
==3863== by 0x4B6DE21:
qpid::broker::SessionHandlerImpl::BasicHandlerImpl::ack(unsigned short,
unsigned long, bool) (SessionHandlerImpl.cpp:427)
==3863== by 0x4B6E8EF:
qpid::broker::SessionHandlerImpl::received(qpid::framing::AMQFrame*)
(SessionHandlerImpl.cpp:101)
==3863== by 0x4CF5534: qpid::sys::LFSessionContext::read()
(LFSessionContext.cpp:63)
==3863== by 0x4CF44DD: qpid::sys::LFProcessor::run() (LFProcessor.cpp:125)
==3863== by 0x4CF70A3: qpid::sys::Thread::runRunnable(apr_thread_t*, void*)
(Thread.cpp:28)
==3863== by 0x51BCF19: start_thread (in /usr/lib/debug/libpthread-2.3.6.so)
==3863== by 0x5A2E5C1: clone (clone.S:112)
The code in question:
25 void AccumulatedAck::update(u_int64_t tag, bool multiple){
26 if(multiple){
27 if(tag > range) range = tag;
28 //else don't care, it is already counted
29 }else if(tag > range){
30 individual.push_back(tag);
31 }
32 }
Not surprisingly, the uses in qpidc's own tests do set the member first:
./tests/TxAckTest.cpp: acked.range = 5;
./tests/AccumulatedAckTest.cpp: ack.range = 5;
What do you think about making AccumulatedAck a class with the
sole constructor requiring an initializer for that member?
That should make it so this can't happen again.
