Disk aggregate and index aggregate could corrupt memory

If the result of a disk aggregate or index aggregate was a variable
size row, roxie heap could get overwritten

Removed the unsafe version of the cloneRow function - which is never
 safe to use in new memory paradigms - to ensure that no other issues
remain. A couple of changes in hthor to avoid the unsafe functions
were unlikely to cause corruptions in practice.

Legacy thor does not appear to use these deprecated functions.

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

common/thorhelper/thorcommon.cpp

@@ -580,24 +580,6 @@ public:
     }
 };
 
-
-//Deprecated - should use the second definition below
-void * cloneRow(IEngineRowAllocator * allocator, const void * row, size32_t &sizeout)
-{
-    IOutputMetaData * meta = allocator->queryOutputMeta();
-    void * ret = allocator->createRow();        
-    sizeout = meta->getRecordSize(row);     // TBD could be better?
-    //GH this may no longer be big enough
-    memcpy(ret, row, sizeout);
-    if (meta->getMetaFlags() & MDFneedserialize)
-    {
-        ChildRowLinkerWalker walker;
-        meta->walkIndirectMembers(static_cast<const byte *>(ret), walker);
-    }
-    //NB: Does not call finalizeRow()...
-    return ret;
-}
-
 //the visitor callback is used to ensure link counts for children are updated.
 size32_t cloneRow(ARowBuilder & rowBuilder, const void * row, IOutputMetaData * meta)
 {

common/thorhelper/thorcommon.ipp

@@ -709,17 +709,8 @@ class NullDiskCallback : public IThorDiskCallback, extends CInterface
     virtual const char * queryLogicalFilename(const void * row) { return NULL; }
 };
 
-extern THORHELPER_API void * cloneRow(IEngineRowAllocator * allocator, const void * row, size32_t &sizeout);
 extern THORHELPER_API size32_t cloneRow(ARowBuilder & rowBuilder, const void * row, IOutputMetaData * meta);
 
-inline void *cloneRow(ICodeContext * ctx, IEngineRowAllocator * allocator, const void * row)
-{
-    // legacy version that doesn't return size
-    size32_t sztmp;
-    return cloneRow(allocator, row, sztmp);
-}
-
-
 //The CThorContiguousRowBuffer is the source for a readAhead call to ensure the entire row
 //is in a contiguous block of memory.  The read() and skip() functions must be implemented
 class THORHELPER_API CThorContiguousRowBuffer : implements IRowDeserializerSource

ecl/hthor/hthorkey.cpp

@@ -954,7 +954,9 @@ const void *CHThorIndexReadActivity::nextInGroup()
                 }
                 try
                 {
-                    return cloneRow(agent.queryCodeContext(), rowAllocator, keyRow);
+                    RtlDynamicRowBuilder rowBuilder(rowAllocator);
+                    size32_t finalSize = cloneRow(rowBuilder, keyRow, outputMeta);
+                    return rowBuilder.finalizeRowClear(finalSize);
                 }
                 catch(IException * e)
                 {
@@ -1040,7 +1042,9 @@ const void *CHThorIndexReadActivity::nextGE(const void * seek, unsigned numField
                 }
                 try
                 {
-                    return cloneRow(agent.queryCodeContext(), rowAllocator, row);
+                    RtlDynamicRowBuilder rowBuilder(rowAllocator);
+                    size32_t finalSize = cloneRow(rowBuilder, row, outputMeta);
+                    return rowBuilder.finalizeRowClear(finalSize);
                 }
                 catch(IException * e)
                 {

roxie/ccd/ccdactivities.cpp

@@ -2408,18 +2408,14 @@ public:
                 }
                 row = m.readDirect(len);
                 if (!finalBuilder.exists())
-                {
-                    size32_t clonedSize;
-                    void * cloned = cloneRow(rowAllocator, row, clonedSize);
-                    finalBuilder.setown(clonedSize, cloned);
-                }
+                    cloneRow(finalBuilder, row, meta);
                 else
                     helper->mergeAggregate(finalBuilder, row);
             }
         }
         if (finalBuilder.exists())
         {
-            size32_t finalSize = meta.getRecordSize(finalBuilder.getSelf());
+            size32_t finalSize = meta.getRecordSize(finalBuilder.getSelf());  // MORE - can probably track it above...
             finalRow.setown(finalBuilder.finalizeRowClear(finalSize));
         }
     }

roxie/ccd/ccdserver.cpp

@@ -20343,9 +20343,7 @@ public:
             else
             {
                  // NOTE need to clone this because going to modify below, could special case 1 row only
-                void * cloned = cloneRow(rowAllocator, firstRow, finalSize);
-                //NB: finalSize is updated by the call above, so don't combine these statements
-                rowBuilder.setown(finalSize, cloned);
+                finalSize = cloneRow(rowBuilder, firstRow, meta);
                 ReleaseRoxieRow(firstRow);
             }
             loop
@@ -22072,9 +22070,7 @@ public:
         else
         {
              // NOTE need to clone this because going to modify below, could special case 1 row only
-            void * cloned = cloneRow(rowAllocator, firstRow, finalSize);
-            //NB: finalSize is updated by the call above, so don't combine these statements
-            rowBuilder.setown(finalSize, cloned);
+            finalSize = cloneRow(rowBuilder, firstRow, meta);
             ReleaseRoxieRow(firstRow);
         }
         loop