Skip to content

Conversation

@luxedo
Copy link
Contributor

@luxedo luxedo commented May 25, 2024

Closes #26137.

Tests capsule name and sets PyErr if not valid:

    if (!PyCapsule_IsValid(handler, MEM_HANDLER_CAPSULE_NAME)) {
        PyErr_SetString(PyExc_ValueError, "Capsule must be named 'mem_handler'")
        return NULL;
    }

@luxedo
Copy link
Contributor Author

luxedo commented May 25, 2024

WIP

Got wrong capsule name in tests:
image

@seberg
Copy link
Member

seberg commented May 25, 2024

If you look at what is failing, you should find that it is failing when SetHandler is called with handler=NULL.

@luxedo luxedo changed the title Enh/26137 PyDataMem_SetHandler check capsule name May 25, 2024
@charris charris changed the title PyDataMem_SetHandler check capsule name BUG: PyDataMem_SetHandler check capsule name May 25, 2024
@luxedo
Copy link
Contributor Author

luxedo commented May 26, 2024

I placed the mem_handler check after the null check and all the tests pass now

@seberg
Copy link
Member

seberg commented May 28, 2024

LGTM, adding that constant seems fine also (it's used in at least one more place, but OK). Would you be up for adding a test? The memory handler tests should be easy to expand to do that I suspect and it's nice to add one, if just to cover the code.

@luxedo
Copy link
Contributor Author

luxedo commented May 31, 2024

I have made a few changes given your last feedback:

  1. Add MEM_HANDLER_CAPSULE_NAME to the header file as a #define following the other example in npy_dlpack:
    #define NPY_DLPACK_CAPSULE_NAME "dltensor"
  2. Rename "mem_handler" to MEM_HANDLER_CAPSULE_NAME in multiarraymodule.c.
  3. Add "mem_handler" check branch test

handler = PyDataMem_DefaultHandler;
}
if (!PyCapsule_IsValid(handler, MEM_HANDLER_CAPSULE_NAME)) {
PyErr_SetString(PyExc_ValueError, "Capsule must be named 'mem_handler'");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to throw this exception? This will only run under the hood right?

If it doesn't make sense, what's the appropriate way to handle this?

P.S: This is my first time reading numpy's C code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this is fine, no reason to bike-shed the error type.

return old;
"""),
("set_wrong_capsule_name_data_policy", "METH_NOARGS", """
PyObject *secret_data =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same struct for the secret_data_policy. Is this ok or should I create a struct just for this test?

Copy link
Member

Choose a reason for hiding this comment

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

That's can be anything here really. But let's rename the object (will just apply the suggestion though).

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks for the test follow up!

return old;
"""),
("set_wrong_capsule_name_data_policy", "METH_NOARGS", """
PyObject *secret_data =
Copy link
Member

Choose a reason for hiding this comment

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

That's can be anything here really. But let's rename the object (will just apply the suggestion though).

handler = PyDataMem_DefaultHandler;
}
if (!PyCapsule_IsValid(handler, MEM_HANDLER_CAPSULE_NAME)) {
PyErr_SetString(PyExc_ValueError, "Capsule must be named 'mem_handler'");
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this is fine, no reason to bike-shed the error type.

@seberg seberg merged commit e41b9c1 into numpy:main Jun 2, 2024
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.

ENH: PyDataMem_SetHandler should check whether the capsule is valid

3 participants