Skip to content

Commit 70f89c2

Browse files
davidwendtclaude
andcommitted
Fix leaked host objects from moving out of unique_ptr::release()
Several places called release() on a std::unique_ptr and then moved from the raw pointer it returned, e.g. std::move(*ptr.release()) or ptr.release()[0]. The device memory was moved out and freed normally, but nothing deleted the small host-side object (rmm::device_buffer, cuda::device_buffer, or cudf::column), so each call leaked it. Move from *ptr instead so the unique_ptr keeps ownership and deletes the moved-from object. This fixes the sites listed in NVIDIA#24926 plus the same pattern in groupby argmin/argmax, the IO column_buffer, the JNI list-overlap code, and the benchmark data generators. Also add a note about this pattern to cpp/REVIEW_GUIDELINES.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f5cb11b commit 70f89c2

19 files changed

Lines changed: 33 additions & 41 deletions

File tree

‎cpp/REVIEW_GUIDELINES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
- CUDA stream/event leaks or improper cleanup
4343
- Missing RAII or proper cleanup, including in exception paths
4444
- Raw owning pointers instead of `std::unique_ptr`, `std::shared_ptr`, `std::reference_wrapper`
45+
- Moving from the raw pointer returned by `std::unique_ptr::release()` (e.g. `std::move(*ptr.release())`, `ptr.release()[0]`, `auto p = contents.data.release();`) leaks the host object; use `std::move(*ptr)` instead. (`table::release()` and `device_uvector::release()` return owning values and are fine.)
4546

4647
### API Breaking Changes
4748
- C++ API changes without proper deprecation warnings

‎cpp/benchmarks/common/generate_input.cu‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -526,11 +526,8 @@ std::unique_ptr<cudf::column> create_random_utf8_string_column(data_profile cons
526526
cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED)),
527527
0};
528528

529-
return cudf::make_strings_column(num_rows,
530-
std::move(offsets),
531-
chars.release(),
532-
null_count,
533-
std::move(*result_bitmask.release()));
529+
return cudf::make_strings_column(
530+
num_rows, std::move(offsets), chars.release(), null_count, std::move(*result_bitmask));
534531
}
535532

536533
// Forward declarations for create_rand_col_fn
@@ -637,7 +634,7 @@ std::unique_ptr<cudf::column> create_random_column(data_profile const& profile,
637634
0};
638635

639636
return std::make_unique<cudf::column>(
640-
dtype, num_rows, data.release(), std::move(*result_bitmask.release()), null_count);
637+
dtype, num_rows, data.release(), std::move(*result_bitmask), null_count);
641638
}
642639

643640
/**
@@ -729,7 +726,7 @@ std::unique_ptr<cudf::column> create_random_column<cudf::struct_view>(data_profi
729726
current_child += children_to_adopt.size();
730727

731728
*current_parent = cudf::make_structs_column(
732-
num_rows, std::move(children_to_adopt), null_count, std::move(*null_mask.release()));
729+
num_rows, std::move(children_to_adopt), null_count, std::move(*null_mask));
733730
}
734731

735732
if (lvl == 1) {
@@ -817,7 +814,7 @@ std::unique_ptr<cudf::column> create_random_column<cudf::list_view>(data_profile
817814
std::move(offsets_column),
818815
std::move(current_child_column),
819816
null_count,
820-
std::move(*null_mask.release()));
817+
std::move(*null_mask));
821818

822819
if (auto const cv = list_column->view();
823820
cudf::has_nonempty_nulls(cv, cudf::get_default_stream())) {
@@ -850,7 +847,7 @@ std::unique_ptr<cudf::column> create_distinct_rows_column(data_profile const& pr
850847
auto null_mask = valid_dist(engine, num_rows);
851848
auto [result_bitmask, null_count] =
852849
cudf::bools_to_mask(cudf::device_span<bool const>(null_mask), cudf::get_default_stream());
853-
col->set_null_mask(std::move(*result_bitmask.release()), null_count);
850+
col->set_null_mask(std::move(*result_bitmask), null_count);
854851
}
855852

856853
return col;

‎cpp/benchmarks/io/text/multibyte_split.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ static cudf::string_scalar create_random_input(int32_t num_chars,
7474

7575
// extract the chars from the returned strings column.
7676
auto input_column_contents = input_column->release();
77-
auto chars_buffer = input_column_contents.data.release();
77+
auto chars_buffer = std::move(input_column_contents.data);
7878

7979
// turn the chars in to a string scalar.
8080
return cudf::string_scalar(std::move(*chars_buffer));

‎cpp/src/copying/copy_range.cu‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ std::unique_ptr<cudf::column> out_of_place_copy_range_dispatch::operator()<cudf:
184184
auto indices_column = std::make_unique<cudf::column>(
185185
new_indices.type(),
186186
new_indices.size(),
187-
std::move(*(target_indices->release().data.release())),
187+
std::move(*(target_indices->release().data)),
188188
cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED, stream, mr),
189189
0);
190190

@@ -194,7 +194,7 @@ std::unique_ptr<cudf::column> out_of_place_copy_range_dispatch::operator()<cudf:
194194
// create column with keys_column and indices_column
195195
return make_dictionary_column(std::move(keys_column),
196196
std::move(indices_column),
197-
std::move(*(target_contents.null_mask.release())),
197+
std::move(*(target_contents.null_mask)),
198198
null_count);
199199
}
200200

‎cpp/src/dictionary/detail/concatenate.cu‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ std::unique_ptr<column> concatenate(host_span<column_view const> columns,
289289
// finally, frankenstein that dictionary column together
290290
return make_dictionary_column(std::move(keys_column),
291291
std::move(indices_column),
292-
std::move(*(contents.null_mask.release())),
292+
std::move(*(contents.null_mask)),
293293
null_count);
294294
}
295295

‎cpp/src/dictionary/dictionary_factories.cu‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ std::unique_ptr<column> make_dictionary_column(std::unique_ptr<column> keys,
141141
if (new_type == indices_type || cudf::size_of(new_type) == cudf::size_of(indices_type)) {
142142
return std::make_unique<column>(new_type,
143143
indices_size,
144-
std::move(*(contents.data.release())),
144+
std::move(*(contents.data)),
145145
cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED),
146146
0);
147147
}
@@ -150,10 +150,8 @@ std::unique_ptr<column> make_dictionary_column(std::unique_ptr<column> keys,
150150
return cudf::detail::cast(cast_view, new_type, stream, mr);
151151
}();
152152

153-
return make_dictionary_column(std::move(keys),
154-
std::move(indices_column),
155-
std::move(*(contents.null_mask.release())),
156-
null_count);
153+
return make_dictionary_column(
154+
std::move(keys), std::move(indices_column), std::move(*(contents.null_mask)), null_count);
157155
}
158156

159157
} // namespace cudf

‎cpp/src/groupby/sort/group_argmax.cu‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ std::unique_ptr<column> group_argmax(column_view const& values,
4646
output.data() // result (must not overlap map)
4747
);
4848
auto null_count = indices_view.null_count();
49-
auto null_mask = indices->release().null_mask.release();
49+
auto null_mask = std::move(indices->release().null_mask);
5050
return std::make_unique<column>(std::move(output), std::move(*null_mask), null_count);
5151
}
5252

‎cpp/src/groupby/sort/group_argmin.cu‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ std::unique_ptr<column> group_argmin(column_view const& values,
4848
output.data() // result (must not overlap map)
4949
);
5050
auto null_count = indices_view.null_count();
51-
auto null_mask = indices->release().null_mask.release();
51+
auto null_mask = std::move(indices->release().null_mask);
5252
return std::make_unique<column>(std::move(output), std::move(*null_mask), null_count);
5353
}
5454

‎cpp/src/interop/from_arrow_host_strings.cu‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ std::unique_ptr<column> from_arrow_string(
6262
std::move(offsets_column),
6363
std::move(chars),
6464
null_count,
65-
std::move(*mask.release()));
65+
std::move(*mask));
6666
}
6767

6868
constexpr int stringview_vector_idx = 1;

‎cpp/src/io/json/write_json.cu‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ std::unique_ptr<column> struct_to_strings(table_view const& strings_columns,
383383
num_rows,
384384
std::make_unique<cudf::column>(
385385
std::move(row_string_offsets), cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED), 0),
386-
std::move(chars_data.release()[0]),
386+
std::move(*chars_data),
387387
0,
388388
cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED));
389389
}
@@ -546,7 +546,7 @@ std::unique_ptr<column> join_list_of_strings(lists_column_view const& lists_stri
546546
num_lists,
547547
std::make_unique<cudf::column>(
548548
std::move(row_string_offsets), cudf::create_null_mask(0, cudf::mask_state::UNALLOCATED), 0),
549-
std::move(chars_data.release()[0]),
549+
std::move(*chars_data),
550550
lists_strings.null_count(),
551551
cudf::detail::copy_bitmask(lists_strings.parent(), stream, mr));
552552
}

0 commit comments

Comments
 (0)