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

[hoodmane]

_ctypes_get_ffi_type never returns ffi_type_void. If the
return type is specified as None, we need set the libffi
return type to void, but just taking the output from
_ctypes_get_ffi_type will make the return type be sint.

This fixes two spots where ctypes accidentally converts
None return type to sint rather than void, causing crashes
on Emscripten targets.

I tested this patch against Pyodide and it fixes test_code.

https://bugs.python.org/issue47197

[jkloth]

Would it be possible to add a test case for this change?

Otherwise, this builds and tests fine on Windows.

Adding @tiran to the loop due to his current work on WASM.

[hoodmane]

add a test case for this change

On wasm targets, there is already test coverage -- test_code and test_ctypes segfault because of this bug. I have this patch on this branch and it fixes the test_code and test_ctypes segfaults:
pyodide/pyodide@2ccb17b

Trying to test on other targets is kind of hard because these calls go directly into ffi_prep_cif, you could build against a custom version of libffi or make a wrapper of ffi_prep_cif that stores the argument specification into some location if a runtime flag is set. I don't think it's worth it.

[tiran]

The change looks correct to me. But I'm not an expert on ctypes code.

Please add a news entry with blurb CLI or blurb-it tool.

[tiran]

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

[tiran]

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]

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]

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.

[hoodmane]

Anything left to do on this?

[hoodmane]

bump

[tiran]

Patch looks good to me.

The PR has not landed yet because I wanted to give the ctypes maintainers more time to review and comment on the change set.

[hoodmane]

Thanks for the reviews @tiran and @jkloth!