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 19:04:53 UTC
C++: Ensure that AccumulatedAck.range is not used uninitialized.
Here's a patch to fix the problem I reported earlier today.
With it, all cpp tests pass (assuming the incoming s/507/530/),
and those in bdbstore no longer trigger a valgrind-used-uninitialized
error.
Note that this patch is minimal, just to fix the bug.
I can easily imagine that someone might want to protect the members
of the new AccumulatedAck class, add accessors, etc., but that
change belongs in a separate delta.
One final note, regarding this change, from below:
- AccumulatedAck ack;
- ack.clear();
+ AccumulatedAck ack(0);
If you'd like to keep the use of ack.clear (perhaps for better
test coverage), let me know and I'll resubmit with this instead:
AccumulatedAck ack(42);
ack.clear();
Here's the patch:
2006-12-08 Jim Meyering <me...@redhat.com>
Ensure that AccumulatedAck.range is not used uninitialized.
* lib/broker/AccumulatedAck.h (AccumulatedAck): Make this a class,
rather than a struct.
(AccumulatedAck::AccumulatedAck): Add a constructor to require
initialization of the "range" member.
* lib/broker/BrokerChannel.cpp (Channel) [accumulatedAck]: Initialize.
* tests/TxAckTest.cpp (TxAckTest) [acked]: Likewise.
* tests/AccumulatedAckTest.cpp (testCovers): Initialize local.
(testUpdateAndConsolidate): Likewise.
Index: lib/broker/AccumulatedAck.h
===================================================================
--- lib/broker/AccumulatedAck.h (revision 495629)
+++ lib/broker/AccumulatedAck.h (working copy)
@@ -31,7 +31,8 @@
* Keeps an accumulated record of acked messages (by delivery
* tag).
*/
- struct AccumulatedAck{
+ class AccumulatedAck {
+ public:
/**
* If not zero, then everything up to this value has been
* acked.
@@ -43,6 +44,7 @@
*/
std::list<u_int64_t> individual;
+ AccumulatedAck(u_int64_t r) : range(r) {}
void update(u_int64_t tag, bool multiple);
void consolidate();
void clear();
Index: lib/broker/BrokerChannel.cpp
===================================================================
--- lib/broker/BrokerChannel.cpp (revision 495629)
+++ lib/broker/BrokerChannel.cpp (working copy)
@@ -40,6 +40,7 @@
prefetchCount(0),
framesize(_framesize),
tagGenerator("sgen"),
+ accumulatedAck(0),
store(_store),
messageBuilder(this, _store, _stagingThreshold),
version(_version){
Index: tests/TxAckTest.cpp
===================================================================
--- tests/TxAckTest.cpp (revision 495629)
+++ tests/TxAckTest.cpp (working copy)
@@ -66,7 +66,7 @@
public:
- TxAckTest() : queue(new Queue("my_queue", false, &store, 0)), op(acked, deliveries, &xid)
+ TxAckTest() : acked(0), queue(new Queue("my_queue", false, &store, 0)), op(acked, deliveries, &xid)
{
for(int i = 0; i < 10; i++){
Message::shared_ptr msg(new Message(0, "exchange", "routing_key", false, false));
Index: tests/AccumulatedAckTest.cpp
===================================================================
--- tests/AccumulatedAckTest.cpp (revision 495629)
+++ tests/AccumulatedAckTest.cpp (working copy)
@@ -36,8 +36,7 @@
public:
void testCovers()
{
- AccumulatedAck ack;
- ack.range = 5;
+ AccumulatedAck ack(5);
ack.individual.push_back(7);
ack.individual.push_back(9);
@@ -56,8 +55,7 @@
void testUpdateAndConsolidate()
{
- AccumulatedAck ack;
- ack.clear();
+ AccumulatedAck ack(0);
ack.update(1, false);
ack.update(3, false);
ack.update(10, false);
Re: C++: Ensure that AccumulatedAck.range is not used
uninitialized.
Posted by Alan Conway <ac...@redhat.com>.
Comitted.
On Fri, 2007-01-12 at 19:04 +0100, Jim Meyering wrote:
> Here's a patch to fix the problem I reported earlier today.
> With it, all cpp tests pass (assuming the incoming s/507/530/),
> and those in bdbstore no longer trigger a valgrind-used-uninitialized
> error.
>
> Note that this patch is minimal, just to fix the bug.
> I can easily imagine that someone might want to protect the members
> of the new AccumulatedAck class, add accessors, etc., but that
> change belongs in a separate delta.
>
> One final note, regarding this change, from below:
> - AccumulatedAck ack;
> - ack.clear();
> + AccumulatedAck ack(0);
> If you'd like to keep the use of ack.clear (perhaps for better
> test coverage), let me know and I'll resubmit with this instead:
>
> AccumulatedAck ack(42);
> ack.clear();
>
> Here's the patch:
>
> 2006-12-08 Jim Meyering <me...@redhat.com>
>
> Ensure that AccumulatedAck.range is not used uninitialized.
> * lib/broker/AccumulatedAck.h (AccumulatedAck): Make this a class,
> rather than a struct.
> (AccumulatedAck::AccumulatedAck): Add a constructor to require
> initialization of the "range" member.
> * lib/broker/BrokerChannel.cpp (Channel) [accumulatedAck]: Initialize.
> * tests/TxAckTest.cpp (TxAckTest) [acked]: Likewise.
> * tests/AccumulatedAckTest.cpp (testCovers): Initialize local.
> (testUpdateAndConsolidate): Likewise.
>
> Index: lib/broker/AccumulatedAck.h
> ===================================================================
> --- lib/broker/AccumulatedAck.h (revision 495629)
> +++ lib/broker/AccumulatedAck.h (working copy)
> @@ -31,7 +31,8 @@
> * Keeps an accumulated record of acked messages (by delivery
> * tag).
> */
> - struct AccumulatedAck{
> + class AccumulatedAck {
> + public:
> /**
> * If not zero, then everything up to this value has been
> * acked.
> @@ -43,6 +44,7 @@
> */
> std::list<u_int64_t> individual;
>
> + AccumulatedAck(u_int64_t r) : range(r) {}
> void update(u_int64_t tag, bool multiple);
> void consolidate();
> void clear();
> Index: lib/broker/BrokerChannel.cpp
> ===================================================================
> --- lib/broker/BrokerChannel.cpp (revision 495629)
> +++ lib/broker/BrokerChannel.cpp (working copy)
> @@ -40,6 +40,7 @@
> prefetchCount(0),
> framesize(_framesize),
> tagGenerator("sgen"),
> + accumulatedAck(0),
> store(_store),
> messageBuilder(this, _store, _stagingThreshold),
> version(_version){
> Index: tests/TxAckTest.cpp
> ===================================================================
> --- tests/TxAckTest.cpp (revision 495629)
> +++ tests/TxAckTest.cpp (working copy)
> @@ -66,7 +66,7 @@
>
> public:
>
> - TxAckTest() : queue(new Queue("my_queue", false, &store, 0)), op(acked, deliveries, &xid)
> + TxAckTest() : acked(0), queue(new Queue("my_queue", false, &store, 0)), op(acked, deliveries, &xid)
> {
> for(int i = 0; i < 10; i++){
> Message::shared_ptr msg(new Message(0, "exchange", "routing_key", false, false));
> Index: tests/AccumulatedAckTest.cpp
> ===================================================================
> --- tests/AccumulatedAckTest.cpp (revision 495629)
> +++ tests/AccumulatedAckTest.cpp (working copy)
> @@ -36,8 +36,7 @@
> public:
> void testCovers()
> {
> - AccumulatedAck ack;
> - ack.range = 5;
> + AccumulatedAck ack(5);
> ack.individual.push_back(7);
> ack.individual.push_back(9);
>
> @@ -56,8 +55,7 @@
>
> void testUpdateAndConsolidate()
> {
> - AccumulatedAck ack;
> - ack.clear();
> + AccumulatedAck ack(0);
> ack.update(1, false);
> ack.update(3, false);
> ack.update(10, false);