From 4f8d9685b45a3f7a0140eb6053fcb096c0f67719 Mon Sep 17 00:00:00 2001 From: Johnny Willemsen Date: Fri, 7 Oct 2022 08:24:40 +0200 Subject: [PATCH] Fixed potential memory leak on an allocation error, use nullptr * ACE/ace/Message_Block.cpp: * ACE/ace/Message_Block.h: --- ACE/ace/Message_Block.cpp | 211 ++++++++++++++++++-------------------- ACE/ace/Message_Block.h | 12 +-- 2 files changed, 107 insertions(+), 116 deletions(-) diff --git a/ACE/ace/Message_Block.cpp b/ACE/ace/Message_Block.cpp index f675a9450f558..4ff96b35f9dac 100644 --- a/ACE/ace/Message_Block.cpp +++ b/ACE/ace/Message_Block.cpp @@ -392,16 +392,16 @@ ACE_Message_Block::ACE_Message_Block (const char *data, if (this->init_i (size, // size MB_DATA, // type - 0, // cont + nullptr, // cont data, // data - 0, // allocator + nullptr, // allocator 0, // locking strategy ACE_Message_Block::DONT_DELETE, // flags priority, // priority ACE_Time_Value::zero, // execution time ACE_Time_Value::max_time, // absolute time of deadline - 0, // data block - 0, // data_block allocator + nullptr, // data block + nullptr, // data_block allocator 0) == -1) // message_block allocator ACELIB_ERROR ((LM_ERROR, ACE_TEXT ("ACE_Message_Block"))); @@ -415,16 +415,16 @@ ACE_Message_Block::ACE_Message_Block (ACE_Allocator *message_block_allocator) if (this->init_i (0, // size MB_DATA, // type - 0, // cont - 0, // data - 0, // allocator + nullptr, // cont + nullptr, // data + nullptr, // allocator 0, // locking strategy ACE_Message_Block::DONT_DELETE, // flags 0, // priority ACE_Time_Value::zero, // execution time ACE_Time_Value::max_time, // absolute time of deadline - 0, // data block - 0, // data_block allocator + nullptr, // data block + nullptr, // data_block allocator message_block_allocator) == -1) // message_block allocator ACELIB_ERROR ((LM_ERROR, ACE_TEXT ("ACE_Message_Block"))); @@ -494,25 +494,24 @@ ACE_Message_Block::init (size_t size, } int -ACE_Message_Block::init (const char *data, - size_t size) +ACE_Message_Block::init (const char *data, size_t size) { ACE_TRACE ("ACE_Message_Block::init"); // Should we also initialize all the other fields, as well? return this->init_i (size, // size MB_DATA, // type - 0, // cont + nullptr, // cont data, // data - 0, // allocator + nullptr, // allocator 0, // locking strategy ACE_Message_Block::DONT_DELETE, // flags 0, // priority ACE_Time_Value::zero, // execution time ACE_Time_Value::max_time, // absolute time of deadline - 0, // data block - 0, // data_block allocator - 0); // message_block allocator + nullptr, // data block + nullptr, // data_block allocator + nullptr); // message_block allocator } ACE_Message_Block::ACE_Message_Block (size_t size, @@ -529,7 +528,7 @@ ACE_Message_Block::ACE_Message_Block (size_t size, ACE_Allocator *data_block_allocator, ACE_Allocator *message_block_allocator) : flags_ (0), - data_block_ (0) + data_block_ (nullptr) { ACE_TRACE ("ACE_Message_Block::ACE_Message_Block"); @@ -554,7 +553,7 @@ ACE_Message_Block::ACE_Message_Block (ACE_Data_Block *data_block, ACE_Message_Block::Message_Flags flags, ACE_Allocator *message_block_allocator) : flags_ (flags), - data_block_ (0) + data_block_ (nullptr) { ACE_TRACE ("ACE_Message_Block::ACE_Message_Block"); @@ -578,7 +577,7 @@ ACE_Message_Block::ACE_Message_Block (ACE_Data_Block *data_block, ACE_Message_Block::ACE_Message_Block (const ACE_Message_Block &mb, size_t align) :flags_ (0), - data_block_ (0) + data_block_ (nullptr) { ACE_TRACE ("ACE_Message_Block::ACE_Message_Block"); @@ -587,10 +586,10 @@ ACE_Message_Block::ACE_Message_Block (const ACE_Message_Block &mb, { if (this->init_i (0, // size MB_NORMAL, // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy 0, // flags 0, // priority ACE_Time_Value::zero, // execution time @@ -616,10 +615,10 @@ ACE_Message_Block::ACE_Message_Block (const ACE_Message_Block &mb, { if (this->init_i (0, // size MB_NORMAL, // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy 0, // flags 0, // priority ACE_Time_Value::zero, // execution time @@ -644,8 +643,7 @@ ACE_Message_Block::ACE_Message_Block (const ACE_Message_Block &mb, #if !defined (ACE_LACKS_CDR_ALIGNMENT) // Get the alignment offset of the incoming ACE_Message_Block - start = ACE_ptr_align_binary (mb.base (), - align); + start = ACE_ptr_align_binary (mb.base (), align); #else start = mb.base (); #endif /* ACE_LACKS_CDR_ALIGNMENT */ @@ -659,9 +657,8 @@ ACE_Message_Block::ACE_Message_Block (const ACE_Message_Block &mb, start, wr_offset); - // Dont move the write pointer, just leave it to the application + // Don't move the write pointer, just leave it to the application // to do what it wants - } #if defined (ACE_LACKS_CDR_ALIGNMENT) ACE_UNUSED_ARG (align); @@ -697,8 +694,8 @@ ACE_Message_Block::init_i (size_t size, ACE_UNUSED_ARG (deadline_time); #endif /* ACE_HAS_TIMED_MESSAGE_BLOCKS */ this->cont_ = msg_cont; - this->next_ = 0; - this->prev_ = 0; + this->next_ = nullptr; + this->prev_ = nullptr; this->message_block_allocator_ = message_block_allocator; @@ -733,7 +730,7 @@ ACE_Message_Block::init_i (size_t size, ACE_TIMEPROBE (ACE_MESSAGE_BLOCK_INIT_I_DB_CTOR); // Message block initialization may fail, while the construction - // succeds. Since ACE may throw no exceptions, we have to do a + // succeeds. Since ACE may throw no exceptions, we have to do a // separate check and clean up, like this: if (db != 0 && db->size () < size) { @@ -762,7 +759,7 @@ ACE_Data_Block::~ACE_Data_Block () ACE_Message_Block::DONT_DELETE)) { this->allocator_strategy_->free ((void *) this->base_); - this->base_ = 0; + this->base_ = nullptr; } } @@ -773,14 +770,14 @@ ACE_Data_Block::release_i () ACE_ASSERT (this->reference_count_ > 0); - ACE_Data_Block *result = 0; + ACE_Data_Block *result = nullptr; // decrement reference count --this->reference_count_; if (this->reference_count_ == 0) // this will cause deletion of this - result = 0; + result = nullptr; else result = this; @@ -792,8 +789,8 @@ ACE_Data_Block::release_no_delete (ACE_Lock *lock) { ACE_TRACE ("ACE_Data_Block::release_no_delete"); - ACE_Data_Block *result = 0; - ACE_Lock *lock_to_be_used = 0; + ACE_Data_Block *result = nullptr; + ACE_Lock *lock_to_be_used = nullptr; // Check if we were passed in a lock if (lock != 0) @@ -843,7 +840,7 @@ ACE_Data_Block::release (ACE_Lock *lock) // We must delete this outside the scope of the locking_strategy_ // since otherwise we'd be trying to "release" through a deleted // pointer! - if (result == 0) + if (result == nullptr) ACE_DES_FREE_THIS (allocator->free, ACE_Data_Block); return result; @@ -908,7 +905,7 @@ ACE_Message_Block::release_i (ACE_Lock *lock) if (this->cont_) { ACE_Message_Block *mb = this->cont_; - ACE_Message_Block *tmp = 0; + ACE_Message_Block *tmp = nullptr; do { @@ -938,12 +935,12 @@ ACE_Message_Block::release_i (ACE_Lock *lock) { if (this->data_block ()->release_no_delete (lock) == 0) result = 1; - this->data_block_ = 0; + this->data_block_ = nullptr; } // We will now commit suicide: this object *must* have come from the // allocator given. - if (this->message_block_allocator_ == 0) + if (this->message_block_allocator_ == nullptr) delete this; else { @@ -960,24 +957,25 @@ ACE_Message_Block::release (ACE_Message_Block *mb) { ACE_TRACE ("ACE_Message_Block::release"); - if (mb != 0) + if (mb != nullptr) return mb->release (); else - return 0; + return nullptr; } ACE_Message_Block::~ACE_Message_Block () { ACE_TRACE ("ACE_Message_Block::~ACE_Message_Block"); - if (ACE_BIT_DISABLED (this->flags_, - ACE_Message_Block::DONT_DELETE)&& + if (ACE_BIT_DISABLED (this->flags_, ACE_Message_Block::DONT_DELETE) && this->data_block ()) - this->data_block ()->release (); + { + this->data_block ()->release (); + } - this->prev_ = 0; - this->next_ = 0; - this->cont_ = 0; + this->prev_ = nullptr; + this->next_ = nullptr; + this->cont_ = nullptr; } ACE_Data_Block * @@ -1012,9 +1010,8 @@ ACE_Message_Block::duplicate () const { ACE_TRACE ("ACE_Message_Block::duplicate"); - ACE_Message_Block *nb_top = 0; - ACE_Message_Block *nb = 0; - + ACE_Message_Block *nb_top = nullptr; + ACE_Message_Block *nb = nullptr; const ACE_Message_Block *current = this; // Increment the reference counts of all the continuation messages. @@ -1031,10 +1028,10 @@ ACE_Message_Block::duplicate () const ACE_NEW_NORETURN (cur_dup, ACE_Message_Block (0, // size ACE_Message_Type (0), // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy 0, // flags current->priority_, // priority ACE_EXECUTION_TIME, @@ -1052,10 +1049,10 @@ ACE_Message_Block::duplicate () const current->message_block_allocator_->malloc (sizeof (ACE_Message_Block))), ACE_Message_Block (0, // size ACE_Message_Type (0), // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy 0, // flags current->priority_, // priority ACE_EXECUTION_TIME, @@ -1108,8 +1105,8 @@ ACE_Message_Block * ACE_Message_Block::duplicate (const ACE_Message_Block *mb) { ACE_TRACE ("ACE_Message_Block::duplicate"); - if (mb == 0) - return 0; + if (mb == nullptr) + return nullptr; else return mb->duplicate (); } @@ -1125,7 +1122,7 @@ ACE_Data_Block::clone (ACE_Message_Block::Message_Flags mask) const // was allocated with max_size_ (and, thus, it's cur_size_ is the same // as max_size_). Maintain the same "has been written" boundary in the // new block by only copying cur_size_ bytes. - if (nb != 0) + if (nb != nullptr) { ACE_OS::memcpy (nb->base_, this->base_, @@ -1145,13 +1142,9 @@ ACE_Data_Block::clone_nocopy (ACE_Message_Block::Message_Flags mask, // You always want to clear this one to prevent memory leaks but you // might add some others later. - const ACE_Message_Block::Message_Flags always_clear = - ACE_Message_Block::DONT_DELETE; - - const size_t newsize = - max_size == 0 ? this->max_size_ : max_size; - - ACE_Data_Block *nb = 0; + const ACE_Message_Block::Message_Flags always_clear = ACE_Message_Block::DONT_DELETE; + const size_t newsize = max_size == 0 ? this->max_size_ : max_size; + ACE_Data_Block *nb = nullptr; ACE_NEW_MALLOC_RETURN (nb, static_cast ( @@ -1166,7 +1159,7 @@ ACE_Data_Block::clone_nocopy (ACE_Message_Block::Message_Flags mask, 0); // Message block initialization may fail while the construction - // succeds. Since as a matter of policy, ACE may throw no + // succeeds. Since as a matter of policy, ACE may throw no // exceptions, we have to do a separate check like this. if (nb != 0 && nb->size () < newsize) { @@ -1176,7 +1169,6 @@ ACE_Data_Block::clone_nocopy (ACE_Message_Block::Message_Flags mask, return 0; } - // Set new flags minus the mask... nb->clr_flags (mask | always_clear); return nb; @@ -1188,40 +1180,39 @@ ACE_Message_Block::clone (Message_Flags mask) const ACE_TRACE ("ACE_Message_Block::clone"); const ACE_Message_Block *old_message_block = this; - ACE_Message_Block *new_message_block = 0; - ACE_Message_Block *new_previous_message_block = 0; - ACE_Message_Block *new_root_message_block = 0; + ACE_Message_Block *new_message_block {}; + ACE_Message_Block *new_previous_message_block {}; + ACE_Message_Block *new_root_message_block {}; do { - // Get a pointer to a "cloned" (will copy the + // Get a pointer to a "cloned" ACE_Data_Block (will copy the // values rather than increment the reference count). ACE_Data_Block *db = old_message_block->data_block ()->clone (mask); - if (db == 0) - return 0; + if (!db) + return nullptr; - if(old_message_block->message_block_allocator_ == 0) + if(old_message_block->message_block_allocator_ == nullptr) { - ACE_NEW_RETURN (new_message_block, - ACE_Message_Block (0, // size - ACE_Message_Type (0), // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy - 0, // flags - old_message_block->priority_, // priority - ACE_EXECUTION_TIME, // execution time - ACE_DEADLINE_TIME, // absolute time to deadline - // Get a pointer to a - // "duplicated" - // (will simply increment the - // reference count). - db, - db->data_block_allocator (), - old_message_block->message_block_allocator_), - 0); + ACE_NEW_NORETURN (new_message_block, + ACE_Message_Block (0, // size + ACE_Message_Type (0), // type + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy + 0, // flags + old_message_block->priority_, // priority + ACE_EXECUTION_TIME, // execution time + ACE_DEADLINE_TIME, // absolute time to deadline + // Get a pointer to a + // "duplicated" + // (will simply increment the + // reference count). + db, + db->data_block_allocator (), + old_message_block->message_block_allocator_)); } else { @@ -1231,13 +1222,13 @@ ACE_Message_Block::clone (Message_Flags mask) const // ACE_NEW_MALLOC_RETURN, there would be a memory leak because the // above db pointer would be left dangling. new_message_block = static_cast (old_message_block->message_block_allocator_->malloc (sizeof (ACE_Message_Block))); - if (new_message_block != 0) + if (new_message_block != nullptr) new (new_message_block) ACE_Message_Block (0, // size ACE_Message_Type (0), // type - 0, // cont - 0, // data - 0, // allocator - 0, // locking strategy + nullptr, // cont + nullptr, // data + nullptr, // allocator + nullptr, // locking strategy 0, // flags old_message_block->priority_, // priority ACE_EXECUTION_TIME, // execution time @@ -1247,10 +1238,10 @@ ACE_Message_Block::clone (Message_Flags mask) const old_message_block->message_block_allocator_); } - if (new_message_block == 0) + if (new_message_block == nullptr) { db->release (); - return 0; + return nullptr; } // Set the read and write pointers in the new to the @@ -1258,15 +1249,15 @@ ACE_Message_Block::clone (Message_Flags mask) const new_message_block->rd_ptr (old_message_block->rd_ptr_); new_message_block->wr_ptr (old_message_block->wr_ptr_); // save the root message block to return - if (new_root_message_block == 0) + if (new_root_message_block == nullptr) new_root_message_block = new_message_block; - if (new_previous_message_block != 0) + if (new_previous_message_block != nullptr) // we're a continuation of the previous block, add ourself to its chain new_previous_message_block->cont_ = new_message_block; new_previous_message_block = new_message_block; old_message_block = old_message_block->cont (); } - while (old_message_block != 0); + while (old_message_block != nullptr); return new_root_message_block; } diff --git a/ACE/ace/Message_Block.h b/ACE/ace/Message_Block.h index 954071060f4f2..39fb2855b500f 100644 --- a/ACE/ace/Message_Block.h +++ b/ACE/ace/Message_Block.h @@ -175,15 +175,15 @@ class ACE_Export ACE_Message_Block */ ACE_Message_Block (size_t size, ACE_Message_Type type = MB_DATA, - ACE_Message_Block *cont = 0, - const char *data = 0, - ACE_Allocator *allocator_strategy = 0, - ACE_Lock *locking_strategy = 0, + ACE_Message_Block *cont = nullptr, + const char *data = nullptr, + ACE_Allocator *allocator_strategy = nullptr, + ACE_Lock *locking_strategy = nullptr, unsigned long priority = ACE_DEFAULT_MESSAGE_BLOCK_PRIORITY, const ACE_Time_Value &execution_time = ACE_Time_Value::zero, const ACE_Time_Value &deadline_time = ACE_Time_Value::max_time, - ACE_Allocator *data_block_allocator = 0, - ACE_Allocator *message_block_allocator = 0); + ACE_Allocator *data_block_allocator = nullptr, + ACE_Allocator *message_block_allocator = nullptr); /** * A copy constructor. This constructor is a bit different. If the