Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 64 additions & 0 deletions Lib/test/test_functools.py
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,70 @@ def test_partial_genericalias(self):
self.assertEqual(alias.__args__, (int,))
self.assertEqual(alias.__parameters__, ())

# Issue 144475
def test_repr_for_segfault(self):
g_partial = None

class Function:
def __init__(self, name):
self.name = name

def __call__(self):
return None

def __repr__(self):
return f"Function({self.name})"

class EvilObject:
def __init__(self, name, is_trigger=False):
self.name = name
self.is_trigger = is_trigger
self.triggered = False

def __repr__(self):
if self.is_trigger and not self.triggered and g_partial is not None:
self.triggered = True
new_args_tuple = (None,)
new_keywords_dict = {"keyword": None}
new_tuple_state = (Function("new_function"), new_args_tuple, new_keywords_dict, None)
g_partial.__setstate__(new_tuple_state)
gc.collect()
return f"EvilObject({self.name})"

trigger = EvilObject("trigger", is_trigger=True)
victim = EvilObject("victim")

p = functools.partial(Function("old_function"), victim, victim=trigger)
g_partial = p
self.assertEqual(repr(g_partial),"functools.partial(Function(old_function), EvilObject(victim), victim=EvilObject(trigger))")

trigger.triggered = False
g_partial = None
p = functools.partial(Function("old_function"), trigger, victim=victim)
g_partial = p
self.assertEqual(repr(g_partial),"functools.partial(Function(old_function), EvilObject(trigger), victim=EvilObject(victim))")


trigger.triggered = False
p = functools.partial(Function("old_function"), trigger, victim)
g_partial = p

trigger.triggered = False
p = functools.partial(Function("old_function"), trigger=trigger, victim=victim)
g_partial = p
self.assertEqual(repr(g_partial),"functools.partial(Function(old_function), trigger=EvilObject(trigger), victim=EvilObject(victim))")

trigger.triggered = False
victim1 = EvilObject("victim")
victim2 = EvilObject("victim")
victim3 = EvilObject("victim")
victim4 = EvilObject("victim")
victim5 = EvilObject("victim")
p = functools.partial(Function("old_function"), trigger, victim1, victim2, victim3, victim4, victim=victim5)
g_partial = p
self.assertEqual(repr(g_partial),"functools.partial(Function(old_function), EvilObject(trigger), EvilObject(victim), EvilObject(victim), EvilObject(victim), EvilObject(victim), victim=EvilObject(victim))")



@unittest.skipUnless(c_functools, 'requires the C _functools module')
class TestPartialC(TestPartial, unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Fixed a bug in :func:`functools.partial` when calling :func:`repr` on a partial
object that could occur when the ``fn``, ``args``, or ``kw`` arguments are modified
during a call to :func:`repr`. Now, calls to :func:`repr` will use the original
arguments when generating the string representation of the partial object.
Subsequent calls will use the updated arguments instead.

45 changes: 26 additions & 19 deletions Modules/_functoolsmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,9 @@ partial_repr(PyObject *self)
partialobject *pto = partialobject_CAST(self);
PyObject *result = NULL;
PyObject *arglist;
PyObject *fn;
PyObject *args;
PyObject *kw;
PyObject *mod;
PyObject *name;
Py_ssize_t i, n;
Expand All @@ -701,52 +704,56 @@ partial_repr(PyObject *self)
return NULL;
return PyUnicode_FromString("...");
}
/* Reference arguments in case they change */
fn = Py_NewRef(pto->fn);
args = Py_NewRef(pto->args);
kw = Py_NewRef(pto->kw);
assert(PyTuple_Check(args));
assert(PyDict_Check(kw));

arglist = Py_GetConstant(Py_CONSTANT_EMPTY_STR);
if (arglist == NULL)
goto done;
goto free_arguments;
/* Pack positional arguments */
assert(PyTuple_Check(pto->args));
n = PyTuple_GET_SIZE(pto->args);
n = PyTuple_GET_SIZE(args);
for (i = 0; i < n; i++) {
Py_SETREF(arglist, PyUnicode_FromFormat("%U, %R", arglist,
PyTuple_GET_ITEM(pto->args, i)));
PyTuple_GET_ITEM(args, i)));
if (arglist == NULL)
goto done;
goto free_arguments;

}
/* Pack keyword arguments */
assert (PyDict_Check(pto->kw));
for (i = 0; PyDict_Next(pto->kw, &i, &key, &value);) {
for (i = 0; PyDict_Next(kw, &i, &key, &value);) {
/* Prevent key.__str__ from deleting the value. */
Py_INCREF(value);
Py_SETREF(arglist, PyUnicode_FromFormat("%U, %S=%R", arglist,
key, value));
Py_DECREF(value);
if (arglist == NULL)
goto done;
goto free_arguments;
}

mod = PyType_GetModuleName(Py_TYPE(pto));
if (mod == NULL) {
goto error;
goto free_arglist;
}
name = PyType_GetQualName(Py_TYPE(pto));
if (name == NULL) {
Py_DECREF(mod);
goto error;
goto free_mod;
}
result = PyUnicode_FromFormat("%S.%S(%R%U)", mod, name, pto->fn, arglist);
Py_DECREF(mod);
result = PyUnicode_FromFormat("%S.%S(%R%U)", mod, name, fn, arglist);
Py_DECREF(name);
free_mod:
Py_DECREF(mod);
free_arglist:
Py_DECREF(arglist);

done:
free_arguments:
Py_DECREF(fn);
Py_DECREF(args);
Py_DECREF(kw);
Py_ReprLeave(self);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this return made sense when the label was 'done'.
Do we still want a side-effect of "return" for a label that "frees arguments"?
Or maybe we keep the "done" label?

Copy link
Author

Choose a reason for hiding this comment

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

I am not exactly sure. I could see arguments for and against adding a done label before the return.

Personally, I don't think it is necessary, as it would never be used. Since the function returns NULL in the case of an error before a new reference is called to the arguments, adding a done label could be a bit misleading. It might suggest that the done label is called if the function needs to return before the fn, args, and kw point to the arguments, as that is the pattern for the other labels.

I would like to get other people's thoughts on this.

Copy link
Author

Choose a reason for hiding this comment

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

Also, since each of the three labels will only be jumped to on an error, a better solution might be to rename each of the labels to the specific error that occurs rather than what gets freed. That way, it makes more sense why the function returns.

error:
Py_DECREF(arglist);
Py_ReprLeave(self);
return NULL;
}

/* Pickle strategy:
Expand Down
Loading