gh-117613: Argument Clinic: ensure that defining_class params are positional-only

[neonene]

This ensures that the AC regards defining_class as positional only, so that _PyArg_UnpackKeywords() can avoid a crash when a keyword mismatches. The crash is not reproducible in test_clinic.c, as it defines Py_BUILD_CORE_MODULE.

cpython/Python/getargs.c

Lines 2515 to 2526 in 25f6ff5

	/* copy keyword args using kwtuple to drive process */
	for (i = Py_MAX((int)nargs, posonly) - Py_SAFE_DOWNCAST(varargssize, Py_ssize_t, int); i < maxargs; i++) {
	PyObject *current_arg;
	if (nkwargs) {
	keyword = PyTuple_GET_ITEM(kwtuple, i - posonly);
	if (kwargs != NULL) {
	if (PyDict_GetItemRef(kwargs, keyword, &current_arg) < 0) {
	goto exit;
	}
	}
	else {
	current_arg = find_keyword(kwnames, kwstack, keyword);

• Issue: Argument Clinic: Unsafe code generation with defining_class and no slash #117613

[erlend-aasland]

The crash is not reproducible in test_clinic.c, as it defines Py_BUILD_CORE_MODULE.

We also have Modules/_testclinic_limited.c which explicitly does not define Py_BUILD_CORE_MODULE.

[neonene]

I failed to make a test. I should have said that Py_BUILD_CORE needs to be defined. Also, it seems to me that the current AC does not support yet METH_FASTCALL, NUM_KEYWORDS, KWTUPLE, if limited_capi is true in clanguage.py.

[erlend-aasland]

I should have said that Py_BUILD_CORE needs to be defined.

In that case, you can use Modules/_testclinic.c which explicitly defines Py_BUILD_CORE_MODULE

[neonene]

As long as the AC generates the check as #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE), both _testclinic and _testclinic_limited would not reproduce a crash. And the check should not be changed in any test cases, right? Then, is there a good built-in module to confirm this issue, or _testclinic_builtin is needed? No reproducer is fine with me, though.

cc @encukou @vstinner: The issue, I think, is that the AC can make a keyword tuple whose size has an extra count of a defining_class parameter, in the clinic/*.c.h file of a builtin module such as _datetime.

[neonene]

An alternative would be not to use a static keyword tuple when defining_class is specified.

[vstinner]

Tests / Check if generated files are up to date (pull_request) Failing after 2m

You can run make clinic && make clinic-tests and commit changed files.

[vstinner]

I propose #117896 to enhance @defining_class tests.

[vstinner]

LGTM.

@erlend-aasland: Do you want to double check?

[vstinner]

You should rebase your PR on main and re-run make clinic && make clinic-tests.

[erlend-aasland]

@vstinner, I think we should backport this to 3.12.

[neonene]

I think I'm not finished rerunning clinic.py.

[erlend-aasland]

I think I'm not finished rerunning clinic.py.

No problem; auto-merge will only work if the CI is green :)

[erlend-aasland]

Thanks, @neonene! I synced with main, just to be on the safe side; when dealing with checked-in generated code it's best to be completely up to date when merging.

[neonene]

Thanks for reviewing and pointing out my mistakes, @vstinner @erlend-aasland.

[miss-islington-app]

Sorry, @neonene and @erlend-aasland, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c520bf9bdf77d43c3d5d95bd08e856759a2abc86 3.12

[erlend-aasland]

@neonene, do you want to do the backport?

[vstinner]

Thanks @neonene for this nice fix. I never noticed that generated code was wrong.