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<T>::Vector(unsigned sz = 10) (Constructor)
2) Vector<T>::Vector(const Vector& src) (Copy constructor)
3) Vector<T>& Vector<T>::operator=(const Vector<T>& obj)
4) void Vector<T>::push(const T & t, unsigned pos)
5) MutexVector<T>::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 T>
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<class T>
-Vector<T>::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<class T>
+Vector<T>::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<class T>
+int
+Vector<T>::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<class T>
Vector<T>::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<class T>
@@ -127,6 +168,8 @@ Vector<T>::operator[](unsigned i) const
template<class T>
T &
Vector<T>::back(){
+ if(m_size==0)
+ abort();
return (* this)[m_size - 1];
}
@@ -134,17 +177,9 @@ template<class T>
int
Vector<T>::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<T>::push_back(const T & t){
}
template<class T>
-void
+int
Vector<T>::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<T>::push(const T & t, unsigned po
}
m_items[pos] = t;
}
+ return 0;
}
template<class T>
T&
Vector<T>::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<T>::clear(){
template<class T>
int
Vector<T>::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<class T>
Vector<T>&
Vector<T>::operator=(const Vector<T>& obj){
if(this != &obj){
clear();
+ const int err = expand(obj.size());
+ if (unlikely(err))
+ abort();
for(unsigned i = 0; i<obj.size(); i++){
- push_back(obj[i]);
+ if (push_back(obj[i]))
+ abort();
}
}
return * this;
@@ -218,12 +269,19 @@ template<class T>
int
Vector<T>::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<cnt; i++)
{
- int ret;
- if ((ret = push_back(src[i])))
- return ret;
+ const int err = push_back(src[i]);
+ if (unlikely(err))
+ return err;
}
return 0;
}
@@ -241,7 +299,8 @@ Vector<T>::equal(const Vector<T>& obj) c
template<class T>
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<T>& operator=(const MutexVector<T>&);
+
T * m_items;
unsigned m_size;
unsigned m_incSize;
unsigned m_arraySize;
};
-template<class T>
-MutexVector<T>::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<class T>
+MutexVector<T>::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<class T>
+int
+MutexVector<T>::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<class T>
@@ -310,6 +403,8 @@ MutexVector<T>::operator[](unsigned i) c
template<class T>
T &
MutexVector<T>::back(){
+ if(m_size==0)
+ abort();
return (* this)[m_size - 1];
}
@@ -318,18 +413,12 @@ int
MutexVector<T>::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<T>::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<NdbQueryOpe
const Vector<NdbQueryOperandImpl*>& 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).
| Thread |
|---|
| • bzr push into mysql-5.5-cluster-7.2 branch (ole.john.aske:3878 to 3880)Bug#13945264 | Ole John Aske | 11 Apr |
