[BUG] Possible regression in handling of default function arguments

[aws-taylor]

Describe the bug

A function with a default parameter value of str() or even int() appears to to get mapped to null. If this value is then later used in a location that performs a null check, such as assigning the value to a dictionary, then the resulting code segfaults. This behavior is new in 3+; in <3, cython appears to correctly map a value such as str() into an empty string, and a value such as int() to a zero.

Looking at the cythonized code, I can see that a value such as '' is treated as an immediate value, whereas str() is treated as a dynamic default argument.

Looking at the cython 0.29 code, I think this is relevant section:

if (!__Pyx_CyFunction_InitDefaults(__pyx_t_1, sizeof(__pyx_defaults), 1)) __PYX_ERR(0, 1, __pyx_L1_error)
  __pyx_t_2 = __Pyx_PyObject_CallNoArg(((PyObject *)(&PyString_Type))); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 1, __pyx_L1_error)
  __Pyx_GOTREF(__pyx_t_2);
  __Pyx_CyFunction_Defaults(__pyx_defaults, __pyx_t_1)->__pyx_arg_this_crashes = __pyx_t_2;
  __Pyx_GIVEREF(__pyx_t_2);
  __pyx_t_2 = 0;

You can see the __pyx_arg_this_crashes getting assigned to the default value of str() (PyString_Type).

In contrast, under 3, it becomes:

  if (!__Pyx_CyFunction_InitDefaults(__pyx_t_2, sizeof(__pyx_defaults), 1)) __PYX_ERR(0, 1, __pyx_L1_error)
  __Pyx_CyFunction_SetDefaultsGetter(__pyx_t_2, __pyx_pf_3wtf_2__defaults__);
  if (PyDict_SetItem(__pyx_d, __pyx_n_s_run_it, __pyx_t_2) < 0) __PYX_ERR(0, 1, __pyx_L1_error)
  __Pyx_DECREF(__pyx_t_2); __pyx_t_2 = 0;
  __pyx_t_2 = __Pyx_PyDict_NewPresized(0); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 1, __pyx_L1_error)
  __Pyx_GOTREF(__pyx_t_2);
  if (PyDict_SetItem(__pyx_d, __pyx_n_s_test, __pyx_t_2) < 0) __PYX_ERR(0, 1, __pyx_L1_error)
  __Pyx_DECREF(__pyx_t_2); __pyx_t_2 = 0;

With:

static PyObject *__pyx_pf_3wtf_2__defaults__(CYTHON_UNUSED PyObject *__pyx_self) {
PyObject *__pyx_r = NULL;
__Pyx_RefNannyDeclarations
PyObject *__pyx_t_1 = NULL;
PyObject *__pyx_t_2 = NULL;
int __pyx_lineno = 0;
const char *__pyx_filename = NULL;
int __pyx_clineno = 0;
__Pyx_RefNannySetupContext("__defaults__", 1);
__Pyx_XDECREF(__pyx_r);
__pyx_t_1 = PyTuple_New(2); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 1, __pyx_L1_error)
__Pyx_GOTREF(__pyx_t_1);
__Pyx_INCREF(((PyObject*)__pyx_kp_s_));
__Pyx_GIVEREF(((PyObject*)__pyx_kp_s_));
if (__Pyx_PyTuple_SET_ITEM(__pyx_t_1, 0, ((PyObject*)__pyx_kp_s_))) __PYX_ERR(0, 1, __pyx_L1_error);
__Pyx_INCREF(__Pyx_CyFunction_Defaults(__pyx_defaults, __pyx_self)->__pyx_arg_this_crashes);
__Pyx_GIVEREF(__Pyx_CyFunction_Defaults(__pyx_defaults, __pyx_self)->__pyx_arg_this_crashes);
if (__Pyx_PyTuple_SET_ITEM(__pyx_t_1, 1, __Pyx_CyFunction_Defaults(__pyx_defaults, __pyx_self)->__pyx_arg_this_crashes)) __PYX_ERR(0, 1, __pyx_L1_error);
__pyx_t_2 = PyTuple_New(2); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 1, __pyx_L1_error)
__Pyx_GOTREF(__pyx_t_2);
__Pyx_GIVEREF(__pyx_t_1);
if (__Pyx_PyTuple_SET_ITEM(__pyx_t_2, 0, __pyx_t_1)) __PYX_ERR(0, 1, __pyx_L1_error);
__Pyx_INCREF(Py_None);
__Pyx_GIVEREF(Py_None);
if (__Pyx_PyTuple_SET_ITEM(__pyx_t_2, 1, Py_None)) __PYX_ERR(0, 1, __pyx_L1_error);
__pyx_t_1 = 0;
__pyx_r = __pyx_t_2;
__pyx_t_2 = 0;
goto __pyx_L0;

This is just copying values out of the defaults object into a tuple though. I can't find anywhere where __pyx_arg_this_crashes is actually set.

Any help you can offer would be appreciated.

Repro:

Using the attached code snippet, invoked as pure-python:

 python3 -c "from snippet import run_it; run_it()"

If I cythonize the attached code using:
cythonize -i -f snippet.py

Then result segfaults.

Code to reproduce the behaviour:

def run_it(this_works='', this_crashes=str()):
  mydict = dict()
  mydict['this_works'] = this_works
  mydict['this_crashes'] = this_crashes
  print(mydict)

Expected behaviour

The code outputs:
{'this_works': '', 'this_crashes': ''}

OS

Linux

Python version

3.9.16

Cython version

3.0.10

Additional context

No response

[da-woods]

The bad commit is 2c8e7b2. Although my feeling is that commit likely isn't the real problem here.

[aws-taylor]

I originally suspected that the issue might have been related to the new str() handling in 3+, especially under different language levels, however the issue still happens if you use int().

[da-woods]

No - there's about 3 different ways of handling defaults depending on how "literal" those defaults are and what type of function it is. It all goes wrong if an optimization accidentally shuffles something between the different mechanisms late in the process.

In this case str() was getting optimized into ''. Turning off the optimizations for defaults would seem like an easy option, but I'm fairly sure some of the optimizations are now required parts of the processes so that would likely break something else.