From: Ole John Aske Date: April 11 2012 9:01am Subject: bzr push into mysql-5.5-cluster-7.2 branch (ole.john.aske:3878 to 3880) Bug#13945264 List-Archive: http://lists.mysql.com/commits/143444 X-Bug: 13945264 Message-Id: <20120411090121.D989E252@fimafeng09.norway.sun.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 3880 Ole John Aske 2012-04-11 Part 2 fix for Bug#13945264: VECTOR TEMPLATE HAS NO SAFE WAY TO SIGNAL 'OUT OF MEMORY' ERRORS Changes usage of Vector in the SPJ API such that memory alloc failured from Vector is now detected by using Vector methods returning an 'int' != 0 instead of checking 'errno == ENOMEM' modified: storage/ndb/src/ndbapi/NdbQueryBuilder.cpp storage/ndb/src/ndbapi/NdbQueryOperation.cpp 3879 Ole John Aske 2012-04-11 Fix for bug#13945264 VECTOR TEMPLATE HAS NO SAFE WAY TO SIGNAL 'OUT OF MEMORY' ERRORS Recommit fixing comments from Magnus B. The Vector template class does internal memory allocation in several methods which are 'void'. As our code is compiled without exceptions, there are no safe way to return an 'out of memory' error from these methods. The questionable methods are: 1) Vector::Vector(unsigned sz = 10) (Constructor) 2) Vector::Vector(const Vector& src) (Copy constructor) 3) Vector& Vector::operator=(const Vector& obj) 4) void Vector::push(const T & t, unsigned pos) 5) MutexVector::MutexVector(unsigned sz = 10) For the C'tors taking a size_t argument the implementation did not correctly handle the case 'size==0'. We wan't to fix this such that - The C'tors should correctly handle 'size==0' such that no initial memory allocation is attempted in these cases, Thus no 'out of memory' allocation can be produced. - Introduce a new 'int ::expand(unsigned sz)' which do an initial alloc of the Vector to the specified size. - Add comments which encourage the user to replace 'Vector(size>0)' constructors with 'Vector(0) + ::expand(n)' constructions. - Add comments which encourage the user to replace copy constructors and assignment operators with empty Vector(0) C'tors and then use 'int Vector::assign()' to fill in the contents. - Changed Vector::push() to return an 'int' used to signal failure. Furthermore, no (deep-) copy constructor or assignment operator was defined for MutexVector such that an incorrect default C'tor would be called. Copy-c'tor and assign operator has now been disallowed. NOTE: This is part 1 of the fix. There will be a part 2 which fix up Vector usage in NdbQueryBuilder and NdbQueryOperator which has been reported as a problem in: https://support.us.oracle.com/oip/faces/secure/ml3/sr/SRDetail.jspx?SRNumber=3-5513205231 ... As these problem has not been reproduced locally we are not sure this will fix the problem though. However it should improve code quality. ****** Part 2 fix for Bug#13945264: VECTOR TEMPLATE HAS NO SAFE WAY TO SIGNAL 'OUT OF MEMORY' ERRORS Changes usage of Vector in the SPJ API such that memory alloc failured from Vector is now detected by using Vector methods returning an 'int' != 0 instead of checking 'errno == ENOMEM' modified: storage/ndb/include/util/Vector.hpp 3878 John David Duncan 2012-04-05 Fix for warnings in CluB. modified: storage/ndb/memcache/src/ndb_error_logger.cc storage/ndb/memcache/src/schedulers/S_sched.cc === modified file 'storage/ndb/include/util/Vector.hpp' --- a/storage/ndb/include/util/Vector.hpp 2011-09-02 09:16:56 +0000 +++ b/storage/ndb/include/util/Vector.hpp 2012-04-11 08:45:06 +0000 @@ -24,7 +24,8 @@ template class Vector { public: - Vector(int sz = 10); + Vector(unsigned sz = 10, unsigned inc_sz = 0); + int expand(unsigned sz); ~Vector(); T& operator[](unsigned i); @@ -32,7 +33,7 @@ public: unsigned size() const { return m_size; }; int push_back(const T &); - void push(const T&, unsigned pos); + int push(const T&, unsigned pos); T& set(T&, unsigned pos, T& fill_obj); T& back(); @@ -63,40 +64,80 @@ private: unsigned m_arraySize; }; -template -Vector::Vector(int i){ - m_items = new T[i]; +/** + * BEWARE: Constructing Vector with initial size > 0 is + * unsafe wrt. catching 'out of memory' errors. + * (C'tor doesn't return error code) + * Instead construct Vector with size==0, and then + * expand() it to the wanted initial size. + */ +template +Vector::Vector(unsigned sz, unsigned inc_sz): + m_items(NULL), + m_size(0), + m_incSize((inc_sz > 0) ? inc_sz : 50), + m_arraySize(0) +{ + if (sz == 0) + return; + + m_items = new T[sz]; if (m_items == NULL) { errno = ENOMEM; - m_size = 0; - m_arraySize = 0; - m_incSize = 0; return; } - m_size = 0; - m_arraySize = i; - m_incSize = 50; + m_arraySize = sz; +} + +template +int +Vector::expand(unsigned sz){ + if (sz <= m_size) + return 0; + + T * tmp = new T[sz]; + if(tmp == NULL) + { + errno = ENOMEM; + return -1; + } + for (unsigned i = 0; i < m_size; i++) + tmp[i] = m_items[i]; + delete[] m_items; + m_items = tmp; + m_arraySize = sz; + return 0; } +/** + * BEWARE: Copy-constructing a Vector is + * unsafe wrt. catching 'out of memory' errors. + * (C'tor doesn't return error code) + * Instead construct empty Vector (size==0), + * and then assign() it the initial contents. + */ template Vector::Vector(const Vector& src): - m_items(new T[src.m_size]), - m_size(src.m_size), + m_items(NULL), + m_size(0), m_incSize(src.m_incSize), - m_arraySize(src.m_size) - + m_arraySize(0) { + const unsigned sz = src.m_size; + if (sz == 0) + return; + + m_items = new T[sz]; if (unlikely(m_items == NULL)){ errno = ENOMEM; - m_size = 0; - m_arraySize = 0; - m_incSize = 0; return; } - for(unsigned i = 0; i < m_size; i++){ + for(unsigned i = 0; i < sz; i++){ m_items[i] = src.m_items[i]; } + m_arraySize = sz; + m_size = sz; } template @@ -127,6 +168,8 @@ Vector::operator[](unsigned i) const template T & Vector::back(){ + if(m_size==0) + abort(); return (* this)[m_size - 1]; } @@ -134,17 +177,9 @@ template int Vector::push_back(const T & t){ if(m_size == m_arraySize){ - T * tmp = new T [m_arraySize + m_incSize]; - if(tmp == NULL) - { - errno = ENOMEM; - return -1; - } - for (unsigned k = 0; k < m_size; k++) - tmp[k] = m_items[k]; - delete[] m_items; - m_items = tmp; - m_arraySize = m_arraySize + m_incSize; + const int err = expand(m_arraySize + m_incSize); + if (unlikely(err)) + return err; } m_items[m_size] = t; m_size++; @@ -152,10 +187,12 @@ Vector::push_back(const T & t){ } template -void +int Vector::push(const T & t, unsigned pos) { - push_back(t); + const int err = push_back(t); + if (unlikely(err)) + return err; if (pos < m_size - 1) { for(unsigned i = m_size - 1; i > pos; i--) @@ -164,13 +201,15 @@ Vector::push(const T & t, unsigned po } m_items[pos] = t; } + return 0; } template T& Vector::set(T & t, unsigned pos, T& fill_obj) { - fill(pos, fill_obj); + if (fill(pos, fill_obj)) + abort(); T& ret = m_items[pos]; m_items[pos] = t; return ret; @@ -196,19 +235,31 @@ Vector::clear(){ template int Vector::fill(unsigned new_size, T & obj){ + const int err = expand(new_size); + if (unlikely(err)) + return err; while(m_size <= new_size) if (push_back(obj)) return -1; return 0; } +/** + * 'operator=' will 'abort()' on 'out of memory' errors. + * You may prefer using ::assign()' which returns + * an error code instead of aborting. + */ template Vector& Vector::operator=(const Vector& obj){ if(this != &obj){ clear(); + const int err = expand(obj.size()); + if (unlikely(err)) + abort(); for(unsigned i = 0; i int Vector::assign(const T* src, unsigned cnt) { + if (getBase() == src) + return 0; // Self-assign is a NOOP + clear(); + const int err = expand(cnt); + if (unlikely(err)) + return err; + for (unsigned i = 0; i::equal(const Vector& obj) c template class MutexVector : public NdbLockable { public: - MutexVector(int sz = 10); + MutexVector(unsigned sz = 10, unsigned inc_sz = 0); + int expand(unsigned sz); ~MutexVector(); T& operator[](unsigned i); @@ -260,26 +319,60 @@ public: int fill(unsigned new_size, T & obj); private: + // Don't allow copy and assignment of MutexVector + MutexVector(const MutexVector&); + MutexVector& operator=(const MutexVector&); + T * m_items; unsigned m_size; unsigned m_incSize; unsigned m_arraySize; }; -template -MutexVector::MutexVector(int i){ - m_items = new T[i]; +/** + * BEWARE: Constructing MutexVector with initial size > 0 is + * unsafe wrt. catching 'out of memory' errors. + * (C'tor doesn't return error code) + * Instead construct MutexVector with size==0, and then + * expand() it to the wanted initial size. + */ +template +MutexVector::MutexVector(unsigned sz, unsigned inc_sz): + m_items(NULL), + m_size(0), + m_incSize((inc_sz > 0) ? inc_sz : 50), + m_arraySize(0) +{ + if (sz == 0) + return; + + m_items = new T[sz]; if (m_items == NULL) { errno = ENOMEM; - m_size = 0; - m_arraySize = 0; - m_incSize = 0; return; } - m_size = 0; - m_arraySize = i; - m_incSize = 50; + m_arraySize = sz; +} + +template +int +MutexVector::expand(unsigned sz){ + if (sz <= m_size) + return 0; + + T * tmp = new T[sz]; + if(tmp == NULL) + { + errno = ENOMEM; + return -1; + } + for (unsigned i = 0; i < m_size; i++) + tmp[i] = m_items[i]; + delete[] m_items; + m_items = tmp; + m_arraySize = sz; + return 0; } template @@ -310,6 +403,8 @@ MutexVector::operator[](unsigned i) c template T & MutexVector::back(){ + if(m_size==0) + abort(); return (* this)[m_size - 1]; } @@ -318,18 +413,12 @@ int MutexVector::push_back(const T & t){ lock(); if(m_size == m_arraySize){ - T * tmp = new T [m_arraySize + m_incSize]; - if (tmp == NULL) + const int err = expand(m_arraySize + m_incSize); + if (unlikely(err)) { - errno = ENOMEM; unlock(); - return -1; + return err; } - for (unsigned k = 0; k < m_size; k++) - tmp[k] = m_items[k]; - delete[] m_items; - m_items = tmp; - m_arraySize = m_arraySize + m_incSize; } m_items[m_size] = t; m_size++; @@ -343,19 +432,13 @@ MutexVector::push_back(const T & t, b if(lockMutex) lock(); if(m_size == m_arraySize){ - T * tmp = new T [m_arraySize + m_incSize]; - if (tmp == NULL) + const int err = expand(m_arraySize + m_incSize); + if (unlikely(err)) { - errno = ENOMEM; if(lockMutex) unlock(); - return -1; + return err; } - for (unsigned k = 0; k < m_size; k++) - tmp[k] = m_items[k]; - delete[] m_items; - m_items = tmp; - m_arraySize = m_arraySize + m_incSize; } m_items[m_size] = t; m_size++; === modified file 'storage/ndb/src/ndbapi/NdbQueryBuilder.cpp' --- a/storage/ndb/src/ndbapi/NdbQueryBuilder.cpp 2012-03-30 12:25:43 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryBuilder.cpp 2012-04-11 09:00:57 +0000 @@ -702,24 +702,7 @@ NdbQueryOperationDef::getIndex() const NdbQueryBuilder* NdbQueryBuilder::create() { NdbQueryBuilderImpl* const impl = new NdbQueryBuilderImpl(); - if (likely (impl != NULL)) - { - if (likely(impl->getNdbError().code == 0)) - { - return &impl->m_interface; - } - else - { - // Probably failed to create Vector instances. - assert(impl->getNdbError().code == Err_MemoryAlloc); - delete impl; - return NULL; - } - } - else - { - return NULL; - } + return (likely(impl!=NULL)) ? &impl->m_interface : NULL; } void NdbQueryBuilder::destroy() @@ -1123,17 +1106,11 @@ NdbQueryBuilder::prepare() NdbQueryBuilderImpl::NdbQueryBuilderImpl() : m_interface(*this), m_error(), - m_operations(), - m_operands(), + m_operations(0), + m_operands(0), m_paramCnt(0), m_hasError(false) -{ - if (errno == ENOMEM) - { - // ENOMEM probably comes from Vector(). - setErrorCode(Err_MemoryAlloc); - } -} +{} NdbQueryBuilderImpl::~NdbQueryBuilderImpl() { @@ -1249,12 +1226,12 @@ NdbQueryDefImpl(const Vector& operands, int& error) : m_interface(*this), - m_operations(operations), - m_operands(operands) + m_operations(0), + m_operands(0) { - if (errno == ENOMEM) + if (m_operations.assign(operations) || m_operands.assign(operands)) { - // Failed to allocate memory for m_operations or m_operands. + // Failed to allocate memory in Vector::assign(). error = Err_MemoryAlloc; return; } @@ -1916,16 +1893,10 @@ NdbQueryOperationDefImpl::NdbQueryOperat m_opNo(opNo), m_internalOpNo(internalOpNo), m_options(options), m_parent(NULL), - m_children(), - m_params(), - m_spjProjection() + m_children(0), + m_params(0), + m_spjProjection(0) { - if (unlikely(errno == ENOMEM)) - { - // Heap allocation in Vector() must have failed. - error = Err_MemoryAlloc; - return; - } if (unlikely(m_internalOpNo >= NDB_SPJ_MAX_TREE_NODES)) { error = QRY_DEFINITION_TOO_LARGE; === modified file 'storage/ndb/src/ndbapi/NdbQueryOperation.cpp' --- a/storage/ndb/src/ndbapi/NdbQueryOperation.cpp 2012-03-30 12:25:43 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryOperation.cpp 2012-04-11 09:00:57 +0000 @@ -3775,7 +3775,7 @@ NdbQueryOperationImpl::NdbQueryOperation m_queryImpl(queryImpl), m_operationDef(def), m_parent(NULL), - m_children(def.getNoOfChildOperations()), + m_children(0), m_maxBatchRows(0), // >0: User specified prefered value, ==0: Use default CFG values m_params(), m_resultBuffer(NULL), @@ -3792,9 +3792,9 @@ NdbQueryOperationImpl::NdbQueryOperation ? Parallelism_max : Parallelism_adaptive), m_rowSize(0xffffffff) { - if (errno == ENOMEM) + if (m_children.expand(def.getNoOfChildOperations())) { - // Memory allocation in Vector() (for m_children) assumed to have failed. + // Memory allocation during Vector::expand() failed. queryImpl.setErrorCode(Err_MemoryAlloc); return; } No bundle (reason: useless for push emails).