Fix read out of bounds in string concatenate

[pentschev]

Description

If data is sufficiently large, fused_concatenate_string_chars_kernel will attempt to read out of bounds and ultimately cause CUDA to raise cudaErrorIllegalAddress. Details on how the issue was encountered are in #13771, although this was an already known problem.

Fixes #13771 .

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[pentschev]

xrefing #13771 (comment) as I was unable to generate tests. For completeness, here's what I wrote there referring missing tests:

In any case, I've now put up a fix for the cudaErrorIllegalAddress in #13838. I confirmed it resolves that problem, but I spent some time (far too much) on it and failed to come up with the appropriate test. If I'm understanding the codeflow correctly, to hit reach that kernel in a way to make it fail we need data that makes the use_fused_kernel_heuristic evaluate to true and the total_bytes must be large enough to require a 64-bit integer for addressing. In my mind, the appropriate test would have a large enough number of columns (failing data has 5000) and the total_bytes must also be large (failing data is 1431647652 bytes long). I would appreciate if someone could help me creating some synthetic test data that we could use to test the kernel actually succeeds.

[bdice]

This appears to be the same fix as #10344. Do we need any other explicit casts for bounds checking, like that PR had? I did not see anything immediately where that would be necessary but may be worth another inspection.

[pentschev]

This appears to be the same fix as #10344. Do we need any other explicit casts for bounds checking, like that PR had? I did not see anything immediately where that would be necessary but may be worth another inspection.

I'm assuming you're referring specifically to this line. AFAIU, this is already covered here, but please correct me if I'm wrong.

[PointKernel]

LGTM

[bdice]

@pentschev My question above seems fine / already covered. Thanks!

[pentschev]

I managed to write a proper test in 25712ec. This is the output if the fix from this PR is reverted:

[ RUN      ] StringColumnTest.ConcatenateColumnViewLarge
CUDA Error detected. cudaErrorIllegalAddress an illegal memory access was encountered
COPYING_TEST: /datasets/pentschev/miniconda3/envs/cudf-invalid-address-src/include/rmm/mr/device/detail/stream_ordered_memory_resource.hpp:253: void rmm::mr::detail::stream_ordered_memory_resource<PoolResource, FreeListType>::do_deallocate(void*, std::size_t, rmm::cuda_stream_view) [with PoolResource = rmm::mr::pool_memory_resource<rmm::mr::cuda_memory_resource>; FreeListType = rmm::mr::detail::coalescing_free_list; std::size_t = long unsigned int]: Assertion `status__ == cudaSuccess' failed.
Aborted (core dumped)

It's a bit of a long running test though (~3.5s), I hope this is ok.

[GregoryKimball]

Would you please use the cudf::thread_index_type alias? Thank you again for diagnosing this issue.

[pentschev]

Thanks @GregoryKimball , addressed that.

[pentschev]

Reviews/questions were addressed and tests are passing. Anything else needed here or could we get it merged?

[PointKernel]

/merge

[PointKernel]

@pentschev If you have write access, feel free to merge the PR on your own after getting enough approvals.