Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] get_primary_cuda_context static map is unsafe in three ways #442

Closed
wence- opened this issue Aug 20, 2024 · 0 comments · Fixed by #472
Closed

[BUG] get_primary_cuda_context static map is unsafe in three ways #442

wence- opened this issue Aug 20, 2024 · 0 comments · Fixed by #472

Comments

@wence-
Copy link
Contributor

wence- commented Aug 20, 2024

  1. The CudaPrimaryContext object has a destructor which calls into the driver (cuDevicePrimaryCtxRelease). But since these objects are stored in a static map, the destructor will run below main, and this is UB per the cuda programming guide
  2. get_primary_cuda_context is not thread-safe since access to the static map is not protected by a lock.
  3. (This came out of reviewing Add resource_ref versions of get/set_current_device_resource rmm#1598)

Since kvikio is header-only, we rely on the linker to disambiguate inline functions that have (or return) static references. To do this, the relevant function must have __attribute__((visibility("default"))). If not, then if kvikio is used in two different DSOs, the function will appear twice, and there will be two static objects.

By default, I believe all rapids libraries are compiled with -fvisiblity-inline-hidden. So kvikio::get_primary_cuda_context (which is inline) will be __attribute((visibility("hidden"))). Thus, if kvikio is used from two separate DSOs, calls from the respective DSOs to get_primary_cuda_context will reference different global statics (since the functions are not the same).

To solve these:

  1. We just need to leak the contexts, I think.
  2. We can add a lock
  3. We should explicitly mark this function as __attribute__((visibility("default")))
@rapids-bot rapids-bot bot closed this as completed in #472 Sep 25, 2024
@rapids-bot rapids-bot bot closed this as completed in 44b3f97 Sep 25, 2024
rapids-bot bot pushed a commit that referenced this issue Oct 7, 2024
We need to export static class methods that returns a reference to a static variable like we do with inline functions: #442

Authors:
  - Mads R. B. Kristensen (https://github.com/madsbk)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant