Skip to content

Require static memory allocation for PyCapsule names #300

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

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

leofang
Copy link
Contributor

@leofang leofang commented Nov 2, 2021

This PR fixes a technical overlook. According to the Python documentation, the only requirement for PyCapsule names is that it must outlive the capsule, but it can be either statically or dynamically allocated. For the latter, the suggestion (not requirement) is to free it when the destructor is launched.

This brings in two issues. First, the DLPack protocol requires the consumer to overwrite the name, but if the name is dynamically allocated, the following operation would raise a compiler warning to the very least:

const char* old_name = PyCapsule_GetName(capsule)
free((void*)old_name);  // lost constness
int result = PyCapsule_SetName(PyCapsule_GetName, new_name);

A further problem is we do not know if free is the correct deallocation routine, which leads to potential crash hazard.

The second problem is a symmetric one w.r.t the 1st: if the consumer also allocates a name dynamically, by the time of destruction it could be freed by the producer's (mismatching) deallocation routine.

This PR suggests a simple fix by asking all libraries to allocate the names statically. This has no practical impact because it's already the status quo, at least in CuPy and PyTorch.

@leofang
Copy link
Contributor Author

leofang commented Nov 2, 2021

cc: @seberg @tqchen

@leofang leofang added this to the v2021 milestone Nov 2, 2021
@kgryte kgryte added the topic: DLPack DLPack. label Nov 3, 2021
@kgryte kgryte requested a review from rgommers November 4, 2021 08:33
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @leofang

@kgryte
Copy link
Contributor

kgryte commented Nov 6, 2021

Will merge. Any revisions can be addressed in follow-up issues/PRs.

@kgryte kgryte merged commit a5489e3 into data-apis:main Nov 6, 2021
@leofang leofang deleted the capsule_name branch November 6, 2021 12:44
@leofang
Copy link
Contributor Author

leofang commented Nov 6, 2021

Thanks all!

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

Successfully merging this pull request may close these issues.

3 participants