You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Jim Meyering <ji...@meyering.net> on 2007/01/12 16:50:43 UTC
C++: ack.range used uninitialized via bdbstore tests
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.
Re: C++: ack.range used uninitialized via bdbstore tests
Posted by Alan Conway <ac...@redhat.com>.
On Fri, 2007-01-12 at 16:50 +0100, Jim Meyering wrote:
> 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.
That sounds right to me.