List:Commits« Previous MessageNext Message »
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
View as plain text  
 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#13945264Ole John Aske11 Apr