Skip to content

Commit

Permalink
Fix AllocateLikeTest gtests reading uninitialized null-mask (#12643)
Browse files Browse the repository at this point in the history
Fixes a bug found in the gtests `AllocateLikeTest.ColumnNumericTestSameSize` and `AllocateLikeTest.ColumnNumericTestSpecifiedSize` where uninitialized column null-masks are used to verify null-counts.
This error was found by running the following in the libcudf build directory:
```
gtests/COPYING_TEST --rmm_mode=cuda
```
The `AllocateLikeTest` tests `cudf::allocate_like()` which produces an uninitialized fixed-width column which is usually filled in by internal functions. The uninitialized column data includes its null-mask. The gtest uses `CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL` to verify the column returned by `allocate_like()` which also includes the `null_count()`. The null-count property returned by `allocate_like()` sets the null-count to `UNKNOWN_NULL_COUNT` which causes a future call to `null_count()` to calculate the count by reading the bits in the null-mask. Since the null-mask is uninitialized (garbage), the null-count would be invalid and likely not match any predictable value.
The gtests are updated to check for appropriate column properties excluding the null-count.

While debugging this issue, the `allocate_like()` logic was found to also attempt to build child columns incorrectly assuming the children will contain the same size and null-mask. Since the `allocate_like()` only supports fixed-width types which do not have children, this logic was removed.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Vukasin Milovanovic (https://github.com/vuule)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #12643
  • Loading branch information
davidwendt authored Jan 31, 2023
1 parent 2b270da commit 6a67e8f
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 19 deletions.
9 changes: 2 additions & 7 deletions cpp/src/copying/copy.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2022, NVIDIA CORPORATION.
* Copyright (c) 2019-2023, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -125,16 +125,11 @@ std::unique_ptr<column> allocate_like(column_view const& input,
CUDF_EXPECTS(is_fixed_width(input.type()), "Expects only fixed-width type column");
mask_state allocate_mask = should_allocate_mask(mask_alloc, input.nullable());

auto op = [&](auto const& child) { return allocate_like(child, size, mask_alloc, stream, mr); };
auto begin = thrust::make_transform_iterator(input.child_begin(), op);
std::vector<std::unique_ptr<column>> children(begin, begin + input.num_children());

return std::make_unique<column>(input.type(),
size,
rmm::device_buffer(size * size_of(input.type()), stream, mr),
detail::create_null_mask(size, allocate_mask, stream, mr),
state_null_count(allocate_mask, input.size()),
std::move(children));
state_null_count(allocate_mask, input.size()));
}

} // namespace detail
Expand Down
50 changes: 38 additions & 12 deletions cpp/tests/copying/utility_tests.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2022, NVIDIA CORPORATION.
* Copyright (c) 2019-2023, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -192,26 +192,52 @@ TYPED_TEST_SUITE(AllocateLikeTest, numeric_types);
TYPED_TEST(AllocateLikeTest, ColumnNumericTestSameSize)
{
// For same size as input
cudf::size_type size = 10;
cudf::mask_state state = cudf::mask_state::ALL_VALID;
auto input = make_numeric_column(cudf::data_type{cudf::type_to_id<TypeParam>()}, size, state);
auto expected = make_numeric_column(
cudf::data_type{cudf::type_to_id<TypeParam>()}, size, cudf::mask_state::ALL_VALID);
cudf::size_type size = 10;

auto input = make_numeric_column(
cudf::data_type{cudf::type_to_id<TypeParam>()}, size, cudf::mask_state::UNALLOCATED);
auto got = cudf::allocate_like(input->view());
CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL(*expected, *got);
CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL(*input, *got);

input = make_numeric_column(
cudf::data_type{cudf::type_to_id<TypeParam>()}, size, cudf::mask_state::ALL_VALID);
got = cudf::allocate_like(input->view());
EXPECT_EQ(input->type(), got->type());
EXPECT_EQ(input->size(), got->size());
EXPECT_EQ(input->nullable(), got->nullable());
EXPECT_EQ(input->num_children(), got->num_children());
// CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL includes checking the null-count property.
// This value will be incorrect since the null mask will contain uninitialized bits
// and the null-count set to UNKNOWN_NULL_COUNT on return from allocate_like().
// This means any subsequent call to null_count() will try to compute the null-count
// using the uninitialized null-mask.
}

TYPED_TEST(AllocateLikeTest, ColumnNumericTestSpecifiedSize)
{
// For same size as input
// For different size as input
cudf::size_type size = 10;
cudf::size_type specified_size = 5;
cudf::mask_state state = cudf::mask_state::ALL_VALID;
auto input = make_numeric_column(cudf::data_type{cudf::type_to_id<TypeParam>()}, size, state);
auto expected = make_numeric_column(
cudf::data_type{cudf::type_to_id<TypeParam>()}, specified_size, cudf::mask_state::ALL_VALID);

auto state = cudf::mask_state::UNALLOCATED;
auto input = make_numeric_column(cudf::data_type{cudf::type_to_id<TypeParam>()}, size, state);
auto expected =
make_numeric_column(cudf::data_type{cudf::type_to_id<TypeParam>()}, specified_size, state);
auto got = cudf::allocate_like(input->view(), specified_size);
CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL(*expected, *got);

input = make_numeric_column(
cudf::data_type{cudf::type_to_id<TypeParam>()}, size, cudf::mask_state::ALL_VALID);
got = cudf::allocate_like(input->view(), specified_size);
EXPECT_EQ(input->type(), got->type());
EXPECT_EQ(specified_size, got->size());
EXPECT_EQ(input->nullable(), got->nullable());
EXPECT_EQ(input->num_children(), got->num_children());
// CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL includes checking the null-count property.
// This value will be incorrect since the null mask will contain uninitialized bits
// and the null-count set to UNKNOWN_NULL_COUNT on return from allocate_like().
// This means any subsequent call to null_count() will try to compute the null-count
// using the uninitialized null-mask.
}

CUDF_TEST_PROGRAM_MAIN()

0 comments on commit 6a67e8f

Please sign in to comment.