Re: C++: Ensure that AccumulatedAck.range is not used uninitialized.

[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);