Skip to content

gh-91353: Fix void return type handling in ctypes (GH-32246) #32246

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.

Sign up for GitHub

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
tiran merged 4 commits into from
Apr 14, 2022
Merged

gh-91353: Fix void return type handling in ctypes (GH-32246) #32246

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f481010
bpo-47197: Fix void return type handling in ctypes
hoodmane Apr 2, 2022
9387bea
Remove '&'
hoodmane Apr 2, 2022
be4c3c4
restype ==> ffi_restype
hoodmane Apr 2, 2022
5b4e9c2
Add blurb
hoodmane Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Modules/_ctypes/callbacks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ CThunkObject *_ctypes_alloc_callback(PyObject *callable,
#endif
result = ffi_prep_cif(&p->cif, cc,
Py_SAFE_DOWNCAST(nargs, Py_ssize_t, int),
_ctypes_get_ffi_type(restype),
p->ffi_restype,

@tiran tiran Apr 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change makes sense. p->ffi_restype is set about 15 lines earlier.

&p->atypes[0]);
if (result != FFI_OK) {
PyErr_Format(PyExc_RuntimeError,
Expand Down
7 changes: 6 additions & 1 deletion Modules/_ctypes/callproc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,12 @@ PyObject *_ctypes_callproc(PPROC pProc,
}
}

rtype = _ctypes_get_ffi_type(restype);
if (restype == Py_None) {
rtype = &ffi_type_void;
} else {
rtype = _ctypes_get_ffi_type(restype);
}
Comment on lines +1212 to +1216

@tiran tiran Apr 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not familiar with ctypes. Would it make sense to check for None in _ctypes_get_ffi_type() rather than here?

ffi_type *_ctypes_get_ffi_type(PyObject *obj)
{
    StgDictObject *dict;
    if (obj == NULL)
        return &ffi_type_sint;
    if (obj == Py_None)
        return &ffi_type_void;
    dict = PyType_stgdict(obj);
    if (dict == NULL)
        return &ffi_type_sint;
...

@hoodmane hoodmane Apr 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is better? I ran into a similar issue when writing the libffi port: _ctypes_get_ffi_type is used for the arguments. Return types need a little bit of special handling, like for instance it doesn't make sense to have an argument of type ffi_type_void. I would argue that it would be safest to do something like:

if (obj == Py_None)
	// Set an error, arguments can't have type None!

but I'm not that familiar with the ctypes interface -- are we supposed to use None if we are not sure about the argument's type? Similarly,

    if (obj == NULL)
        return &ffi_type_sint;

looks kind of scary to me. Why did we pass in NULL to this function? If this function set exceptions in these weird cases and they were handled as appropriate at the call site, the bug I'm fixing here probably wouldn't have happened in the first place.

@jkloth jkloth Apr 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The NULL value is used to indicate not-set and is quite pervasive in the ctypes codebase. Function prototypes are not required to supply argtypes or restype. It seems _ctypes_get_ffi_type() is designed for arguments so the additional checking for Py_None outside of it would be correct.


resbuf = alloca(max(rtype->size, sizeof(ffi_arg)));

#ifdef _Py_MEMORY_SANITIZER
Expand Down