Add a style check for unsafe code

This commit is contained in:
Alexey Milovidov 2023-03-18 03:29:14 +01:00
parent 0b641fcead
commit aea421cc9b
5 changed files with 27 additions and 24 deletions

View File

@ -404,19 +404,19 @@ private:
} }
if (is_column_const[0]) if (is_column_const[0])
col_agg_func = typeid_cast<const ColumnAggregateFunction*>(typeid_cast<const ColumnConst*>(column_ptrs[0])->getDataColumnPtr().get()); col_agg_func = &typeid_cast<const ColumnAggregateFunction &>(*typeid_cast<const ColumnConst &>(*column_ptrs[0]).getDataColumnPtr());
else else
col_agg_func = typeid_cast<const ColumnAggregateFunction*>(column_ptrs[0]); col_agg_func = &typeid_cast<const ColumnAggregateFunction &>(*column_ptrs[0]);
container0 = &col_agg_func->getData(); container0 = &col_agg_func->getData();
if (is_column_const[1]) if (is_column_const[1])
container1 = &typeid_cast<const ColumnUInt64*>(typeid_cast<const ColumnConst*>(column_ptrs[1])->getDataColumnPtr().get())->getData(); container1 = &typeid_cast<const ColumnUInt64 &>(*typeid_cast<const ColumnConst &>(column_ptrs[1]).getDataColumnPtr()).getData();
else else
container1 = &typeid_cast<const ColumnUInt64*>(column_ptrs[1])->getData(); container1 = &typeid_cast<const ColumnUInt64 &>(*column_ptrs[1]).getData();
if (is_column_const[2]) if (is_column_const[2])
container2 = &typeid_cast<const ColumnUInt64*>(typeid_cast<const ColumnConst*>(column_ptrs[2])->getDataColumnPtr().get())->getData(); container2 = &typeid_cast<const ColumnUInt64 &>(*typeid_cast<const ColumnConst &>(column_ptrs[2]).getDataColumnPtr()).getData();
else else
container2 = &typeid_cast<const ColumnUInt64*>(column_ptrs[2])->getData(); container2 = &typeid_cast<const ColumnUInt64 &>(*column_ptrs[2]).getData();
auto col_to = ColumnAggregateFunction::create(col_agg_func->getAggregateFunction()); auto col_to = ColumnAggregateFunction::create(col_agg_func->getAggregateFunction());
col_to->reserve(input_rows_count); col_to->reserve(input_rows_count);
@ -587,29 +587,29 @@ private:
if (is_column_const[0]) if (is_column_const[0])
{ {
col_agg_func = typeid_cast<const ColumnAggregateFunction*>(typeid_cast<const ColumnConst*>(column_ptrs[0])->getDataColumnPtr().get()); col_agg_func = &typeid_cast<const ColumnAggregateFunction &>(*typeid_cast<const ColumnConst &>(*column_ptrs[0]).getDataColumnPtr());
} }
else else
{ {
col_agg_func = typeid_cast<const ColumnAggregateFunction*>(column_ptrs[0]); col_agg_func = &typeid_cast<const ColumnAggregateFunction &>(*column_ptrs[0]);
} }
container0 = &col_agg_func->getData(); container0 = &col_agg_func->getData();
if (is_column_const[1]) if (is_column_const[1])
array1 = typeid_cast<const ColumnArray*>(typeid_cast<const ColumnConst*>(column_ptrs[1])->getDataColumnPtr().get()); array1 = &typeid_cast<const ColumnArray &>(*typeid_cast<const ColumnConst &>(*column_ptrs[1]).getDataColumnPtr());
else else
array1 = typeid_cast<const ColumnArray *>(column_ptrs[1]); array1 = &typeid_cast<const ColumnArray &>(*column_ptrs[1]);
const ColumnArray::Offsets & from_offsets = array1->getOffsets(); const ColumnArray::Offsets & from_offsets = array1->getOffsets();
const ColumnVector<UInt64>::Container & from_container = typeid_cast<const ColumnVector<UInt64> *>(&array1->getData())->getData(); const ColumnVector<UInt64>::Container & from_container = typeid_cast<const ColumnVector<UInt64> &>(array1->getData()).getData();
if (is_column_const[2]) if (is_column_const[2])
array2 = typeid_cast<const ColumnArray*>(typeid_cast<const ColumnConst*>(column_ptrs[2])->getDataColumnPtr().get()); array2 = &typeid_cast<const ColumnArray &>(*typeid_cast<const ColumnConst &>(*column_ptrs[2]).getDataColumnPtr());
else else
array2 = typeid_cast<const ColumnArray *>(column_ptrs[2]); array2 = &typeid_cast<const ColumnArray &>(*column_ptrs[2]);
const ColumnArray::Offsets & to_offsets = array2->getOffsets(); const ColumnArray::Offsets & to_offsets = array2->getOffsets();
const ColumnVector<UInt64>::Container & to_container = typeid_cast<const ColumnVector<UInt64> *>(&array2->getData())->getData(); const ColumnVector<UInt64>::Container & to_container = typeid_cast<const ColumnVector<UInt64> &>(array2->getData()).getData();
auto col_to = ColumnAggregateFunction::create(col_agg_func->getAggregateFunction()); auto col_to = ColumnAggregateFunction::create(col_agg_func->getAggregateFunction());
col_to->reserve(input_rows_count); col_to->reserve(input_rows_count);
@ -911,9 +911,9 @@ private:
is_column_const[0] = isColumnConst(*column_ptrs[0]); is_column_const[0] = isColumnConst(*column_ptrs[0]);
if (is_column_const[0]) if (is_column_const[0])
container0 = &typeid_cast<const ColumnAggregateFunction*>(typeid_cast<const ColumnConst*>(column_ptrs[0])->getDataColumnPtr().get())->getData(); container0 = &typeid_cast<const ColumnAggregateFunction &>(*typeid_cast<const ColumnConst &>(*column_ptrs[0]).getDataColumnPtr()).getData();
else else
container0 = &typeid_cast<const ColumnAggregateFunction*>(column_ptrs[0])->getData(); container0 = &typeid_cast<const ColumnAggregateFunction &>(*column_ptrs[0]).getData();
// we can always cast the second column to ColumnUInt64 // we can always cast the second column to ColumnUInt64
auto uint64_column = castColumn(arguments[1], std::make_shared<DataTypeUInt64>()); auto uint64_column = castColumn(arguments[1], std::make_shared<DataTypeUInt64>());
@ -921,9 +921,9 @@ private:
is_column_const[1] = isColumnConst(*column_ptrs[1]); is_column_const[1] = isColumnConst(*column_ptrs[1]);
if (is_column_const[1]) if (is_column_const[1])
container1 = &typeid_cast<const ColumnUInt64*>(typeid_cast<const ColumnConst*>(column_ptrs[1])->getDataColumnPtr().get())->getData(); container1 = &typeid_cast<const ColumnUInt64 &>(*typeid_cast<const ColumnConst &>(*column_ptrs[1]).getDataColumnPtr()).getData();
else else
container1 = &typeid_cast<const ColumnUInt64*>(column_ptrs[1])->getData(); container1 = &typeid_cast<const ColumnUInt64 &>(*column_ptrs[1]).getData();
for (size_t i = 0; i < input_rows_count; ++i) for (size_t i = 0; i < input_rows_count; ++i)
{ {

View File

@ -1216,7 +1216,7 @@ private:
template <bool first> template <bool first>
void executeArray(const KeyType & key, const IDataType * type, const IColumn * column, typename ColumnVector<ToType>::Container & vec_to) const void executeArray(const KeyType & key, const IDataType * type, const IColumn * column, typename ColumnVector<ToType>::Container & vec_to) const
{ {
const IDataType * nested_type = typeid_cast<const DataTypeArray *>(type)->getNestedType().get(); const IDataType * nested_type = typeid_cast<const DataTypeArray &>(*type).getNestedType().get();
if (const ColumnArray * col_from = checkAndGetColumn<ColumnArray>(column)) if (const ColumnArray * col_from = checkAndGetColumn<ColumnArray>(column))
{ {

View File

@ -313,7 +313,7 @@ FunctionArrayIntersect::UnpackedArrays FunctionArrayIntersect::prepareArrays(
{ {
arg.is_const = true; arg.is_const = true;
argument_column = argument_column_const->getDataColumnPtr().get(); argument_column = argument_column_const->getDataColumnPtr().get();
initial_column = typeid_cast<const ColumnConst *>(initial_column)->getDataColumnPtr().get(); initial_column = &typeid_cast<const ColumnConst &>(*initial_column).getDataColumn();
} }
if (const auto * argument_column_array = typeid_cast<const ColumnArray *>(argument_column)) if (const auto * argument_column_array = typeid_cast<const ColumnArray *>(argument_column))
@ -324,13 +324,13 @@ FunctionArrayIntersect::UnpackedArrays FunctionArrayIntersect::prepareArrays(
arg.offsets = &argument_column_array->getOffsets(); arg.offsets = &argument_column_array->getOffsets();
arg.nested_column = &argument_column_array->getData(); arg.nested_column = &argument_column_array->getData();
initial_column = &typeid_cast<const ColumnArray *>(initial_column)->getData(); initial_column = &typeid_cast<const ColumnArray &>(*initial_column).getData();
if (const auto * column_nullable = typeid_cast<const ColumnNullable *>(arg.nested_column)) if (const auto * column_nullable = typeid_cast<const ColumnNullable *>(arg.nested_column))
{ {
arg.null_map = &column_nullable->getNullMapData(); arg.null_map = &column_nullable->getNullMapData();
arg.nested_column = &column_nullable->getNestedColumn(); arg.nested_column = &column_nullable->getNestedColumn();
initial_column = &typeid_cast<const ColumnNullable *>(initial_column)->getNestedColumn(); initial_column = &typeid_cast<const ColumnNullable &>(*initial_column).getNestedColumn();
} }
/// In case column was casted need to create overflow mask for integer types. /// In case column was casted need to create overflow mask for integer types.

View File

@ -1111,12 +1111,12 @@ void ExpressionActionsChain::JoinStep::finalize(const NameSet & required_output_
ActionsDAGPtr & ExpressionActionsChain::Step::actions() ActionsDAGPtr & ExpressionActionsChain::Step::actions()
{ {
return typeid_cast<ExpressionActionsStep *>(this)->actions_dag; return typeid_cast<ExpressionActionsStep &>(*this).actions_dag;
} }
const ActionsDAGPtr & ExpressionActionsChain::Step::actions() const const ActionsDAGPtr & ExpressionActionsChain::Step::actions() const
{ {
return typeid_cast<const ExpressionActionsStep *>(this)->actions_dag; return typeid_cast<const ExpressionActionsStep &>(*this).actions_dag;
} }
} }

View File

@ -395,3 +395,6 @@ for i in "${ROOT_PATH}"/tests/integration/test_*; do FILE="${i}/__init__.py"; [
# A small typo can lead to debug code in release builds, see https://github.com/ClickHouse/ClickHouse/pull/47647 # A small typo can lead to debug code in release builds, see https://github.com/ClickHouse/ClickHouse/pull/47647
find $ROOT_PATH/{src,programs,utils} -name '*.h' -or -name '*.cpp' | xargs grep -l -F '#ifdef NDEBUG' | xargs -I@FILE awk '/#ifdef NDEBUG/ { inside = 1; dirty = 1 } /#endif/ { if (inside && dirty) { print "File @FILE has suspicious #ifdef NDEBUG, possibly confused with #ifndef NDEBUG" }; inside = 0 } /#else/ { dirty = 0 }' @FILE find $ROOT_PATH/{src,programs,utils} -name '*.h' -or -name '*.cpp' | xargs grep -l -F '#ifdef NDEBUG' | xargs -I@FILE awk '/#ifdef NDEBUG/ { inside = 1; dirty = 1 } /#endif/ { if (inside && dirty) { print "File @FILE has suspicious #ifdef NDEBUG, possibly confused with #ifndef NDEBUG" }; inside = 0 } /#else/ { dirty = 0 }' @FILE
# If a user is doing dynamic or typeid cast with a pointer, and immediately dereferencing it, it is unsafe.
find $ROOT_PATH/{src,programs,utils} -name '*.h' -or -name '*.cpp' | xargs grep --line-number -P '(dynamic|typeid)_cast<[^>]+\*>\([^\(\)]+\)->' | grep -P '.' && echo "It's suspicious when you are doing a dynamic_cast or typeid_cast with a pointer and immediately dereferencing it. Use references instead of pointers or check a pointer to nullptr."