Merge pull request #10638 from ClickHouse/fix-ubsan-hashtable

Fix UBSan report in HashTable

contrib/zstd-cmake/CMakeLists.txt

@@ -128,3 +128,4 @@ ENDIF (ZSTD_LEGACY_SUPPORT)
 ADD_LIBRARY(zstd ${Sources} ${Headers})
 
 target_include_directories (zstd PUBLIC ${LIBRARY_DIR})
+target_compile_options(zstd PRIVATE -fno-sanitize=undefined)

src/AggregateFunctions/AggregateFunctionGroupUniqArray.h

 #pragma once
 
+#include <cassert>
+
 #include <IO/WriteHelpers.h>
 #include <IO/ReadHelpers.h>
 
@@ -139,7 +141,8 @@ static void deserializeAndInsertImpl(StringRef str, IColumn & data_to);
  */
 template <bool is_plain_column = false, typename Tlimit_num_elem = std::false_type>
 class AggregateFunctionGroupUniqArrayGeneric
-    : public IAggregateFunctionDataHelper<AggregateFunctionGroupUniqArrayGenericData, AggregateFunctionGroupUniqArrayGeneric<is_plain_column, Tlimit_num_elem>>
+    : public IAggregateFunctionDataHelper<AggregateFunctionGroupUniqArrayGenericData,
+        AggregateFunctionGroupUniqArrayGeneric<is_plain_column, Tlimit_num_elem>>
 {
     DataTypePtr & input_data_type;
 
@@ -158,6 +161,7 @@ class AggregateFunctionGroupUniqArrayGeneric
         {
             const char * begin = nullptr;
             StringRef serialized = column.serializeValueIntoArena(row_num, arena, begin);
+            assert(serialized.data != nullptr);
             return SerializedKeyHolder{serialized, arena};
         }
     }
@@ -204,9 +208,7 @@ public:
         //TODO: set.reserve(size);
 
         for (size_t i = 0; i < size; ++i)
-        {
             set.insert(readStringBinaryInto(*arena, buf));
-        }
     }
 
     void add(AggregateDataPtr place, const IColumn ** columns, size_t row_num, Arena * arena) const override
@@ -249,9 +251,7 @@ public:
         offsets_to.push_back(offsets_to.back() + set.size());
 
         for (auto & elem : set)
-        {
             deserializeAndInsert(elem.getValue(), data_to);
-        }
     }
 };
 

src/Columns/ColumnArray.cpp

@@ -159,13 +159,17 @@ void ColumnArray::insertData(const char * pos, size_t length)
 
     size_t field_size = data->sizeOfValueIfFixed();
 
-    const char * end = pos + length;
     size_t elems = 0;
-    for (; pos + field_size <= end; pos += field_size, ++elems)
-        data->insertData(pos, field_size);
 
-    if (pos != end)
-        throw Exception("Incorrect length argument for method ColumnArray::insertData", ErrorCodes::BAD_ARGUMENTS);
+    if (length)
+    {
+        const char * end = pos + length;
+        for (; pos + field_size <= end; pos += field_size, ++elems)
+            data->insertData(pos, field_size);
+
+        if (pos != end)
+            throw Exception("Incorrect length argument for method ColumnArray::insertData", ErrorCodes::BAD_ARGUMENTS);
+    }
 
     getOffsets().push_back(getOffsets().back() + elems);
 }

src/Common/HashTable/FixedHashTable.h

@@ -274,9 +274,21 @@ public:
         return iterator(this, ptr);
     }
 
-    const_iterator end() const { return const_iterator(this, buf + NUM_CELLS); }
-    const_iterator cend() const { return end(); }
-    iterator end() { return iterator(this, buf + NUM_CELLS); }
+    const_iterator end() const
+    {
+        /// Avoid UBSan warning about adding zero to nullptr. It is valid in C++20 (and earlier) but not valid in C.
+        return const_iterator(this, buf ? buf + NUM_CELLS : buf);
+    }
+
+    const_iterator cend() const
+    {
+        return end();
+    }
+
+    iterator end()
+    {
+        return iterator(this, buf ? buf + NUM_CELLS : buf);
+    }
 
 
 public:
@@ -325,12 +337,17 @@ public:
         Cell::State::write(wb);
         DB::writeVarUInt(m_size, wb);
 
+        if (!buf)
+            return;
+
         for (auto ptr = buf, buf_end = buf + NUM_CELLS; ptr < buf_end; ++ptr)
+        {
             if (!ptr->isZero(*this))
             {
                 DB::writeVarUInt(ptr - buf);
                 ptr->write(wb);
             }
+        }
     }
 
     void writeText(DB::WriteBuffer & wb) const
@@ -338,6 +355,9 @@ public:
         Cell::State::writeText(wb);
         DB::writeText(m_size, wb);
 
+        if (!buf)
+            return;
+
         for (auto ptr = buf, buf_end = buf + NUM_CELLS; ptr < buf_end; ++ptr)
         {
             if (!ptr->isZero(*this))

src/Common/HashTable/HashTable.h

@@ -710,9 +710,21 @@ public:
         return iterator(this, ptr);
     }
 
-    const_iterator end() const         { return const_iterator(this, buf + grower.bufSize()); }
-    const_iterator cend() const        { return end(); }
-    iterator end()                     { return iterator(this, buf + grower.bufSize()); }
+    const_iterator end() const
+    {
+        /// Avoid UBSan warning about adding zero to nullptr. It is valid in C++20 (and earlier) but not valid in C.
+        return const_iterator(this, buf ? buf + grower.bufSize() : buf);
+    }
+
+    const_iterator cend() const
+    {
+        return end();
+    }
+
+    iterator end()
+    {
+        return iterator(this, buf ? buf + grower.bufSize() : buf);
+    }
 
 
 protected:
@@ -935,6 +947,9 @@ public:
         if (this->hasZero())
             this->zeroValue()->write(wb);
 
+        if (!buf)
+            return;
+
         for (auto ptr = buf, buf_end = buf + grower.bufSize(); ptr < buf_end; ++ptr)
             if (!ptr->isZero(*this))
                 ptr->write(wb);
@@ -951,6 +966,9 @@ public:
             this->zeroValue()->writeText(wb);
         }
 
+        if (!buf)
+            return;
+
         for (auto ptr = buf, buf_end = buf + grower.bufSize(); ptr < buf_end; ++ptr)
         {
             if (!ptr->isZero(*this))

src/Common/HashTable/HashTableKeyHolder.h

@@ -98,8 +98,7 @@ inline void ALWAYS_INLINE keyHolderDiscardKey(DB::ArenaKeyHolder &)
 namespace DB
 {
 
-/**
-  * SerializedKeyHolder is a key holder for a StringRef key that is already
+/** SerializedKeyHolder is a key holder for a StringRef key that is already
   * serialized to an Arena. The key must be the last allocation in this Arena,
   * and is discarded by rolling back the allocation.
   */