bpo-29800: Fix crashes in partial.repr if the keys of partial.keywords are not strings #649

MSeifert04 · 2017-03-12T21:13:06Z

This is my first contribution for CPython and I must admit I haven't read through all the developer guide.

I signed the CLA (probably still in review) but if you notice anything else that needs to be done please let me know.

the-knights-who-say-ni · 2017-03-12T21:13:07Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:

Sign the PSF contributor agreement. The "bugs.python.org username" requested by the form is the "Login name" field in "Your Details" at b.p.o
Wait at least one US business day and then check the "Contributor form received entry under "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-03-12T21:13:08Z

@MSeifert04, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @rhettinger, @ncoghlan, @ambv and @abalkin to be potential reviewers.

…ial.__repr__

serhiy-storchaka · 2017-03-14T14:29:19Z

Lib/test/test_functools.py

+        # Adding a non-string/unicode keyword to partial kwargs
+        p.keywords[1] = 10
+        self.assertIn('1=10', repr(p))
+        try:


Just use assertRaises().

That failed because assertRaises needs the __name__ of the class/function. But _functools.partial doesn't have a __name__.

with self.assertRaises(TypeError): p()

serhiy-storchaka · 2017-03-14T14:38:21Z

Lib/test/test_functools.py

+        p = self.partial(capture)
+        # Adding a non-string/unicode keyword to partial kwargs
+        p.keywords[1] = 10
+        self.assertIn('1=10', repr(p))


This test is fragile. Since 1=10 is not valid Python syntax, more comprehensive implementation can return partial(<foo>, **{1: 10}).

I would test that the repr() of the partial object contains either str() or repr() of the key and the repr() of the value.

p.keywords[123] = 'value' r = repr(p) self.assertIn('123', r) self.assertIn("'value'", r)

serhiy-storchaka · 2017-03-14T14:40:19Z

Lib/test/test_functools.py

+        else:
+            self.fail('partial object allowed passing non-string keywords')
+
+        # Deleting the key-value pair during key formatting shouldn't segfault.


This should be a separate test method.

serhiy-storchaka · 2017-03-14T14:41:57Z

Lib/test/test_functools.py

+
+        # Deleting the key-value pair during key formatting shouldn't segfault.
+        class MutatesYourDict(object):
+            def __init__(self, dct):


The __init__ method and the dct attribute are not needed. Just use p.keywords instead of self.dct.

serhiy-storchaka · 2017-03-14T14:43:19Z

Lib/test/test_functools.py

+            def __str__(self):
+                del self.dct[self]
+                return 'astr'
+            def __repr__(self):


This is not needed as well as wrapping the value with MutatesYourDict.

serhiy-storchaka · 2017-03-14T14:44:11Z

Lib/test/test_functools.py

+
+        p = partial(capture)
+        p.keywords[MutatesYourDict(p.keywords)] = MutatesYourDict(p.keywords)
+        self.assertIn('astr=arepr', repr(p))


This test is fragile. See the comment above.

serhiy-storchaka · 2017-03-14T14:46:08Z

Lib/test/test_functools.py

+                return 'arepr'
+
+        p = partial(capture)
+        p.keywords[MutatesYourDict(p.keywords)] = MutatesYourDict(p.keywords)


I think that wrapping the value with MutatesYourDict is not needed. Just use some unique value with predicable repr. For example ['value'].

Check that this test is failed when don't add increfs.

Should I reorder the commits to add the tests first to show they are segfaulting before doing the commit that fixes them? Or do you mean I should check it locally?

Check it locally. Remove increfs, check that the test is failed/crashed, add increfs, check that the same test is passed now.

Just to report back: It segfaulted (on Windows 10).

serhiy-storchaka · 2017-03-14T14:54:53Z

Modules/_functoolsmodule.c

@@ -253,8 +253,13 @@ partial_repr(partialobject *pto)
    /* Pack keyword arguments */
    assert (PyDict_Check(pto->kw));
    for (i = 0; PyDict_Next(pto->kw, &i, &key, &value);) {
-        Py_SETREF(arglist, PyUnicode_FromFormat("%U, %U=%R", arglist,
+        /* Prevent key.__str__ from deleting key or value during formatting. */
+        Py_INCREF(key);


I think that increfing the key is not needed.

Yes, indeed. I have no problems without that INCREF and I will remove that. That part was copied from dict_repr because I thought what works there needs to work here as well.

serhiy-storchaka · 2017-03-14T14:56:15Z

Lib/test/test_functools.py

+            def __init__(self, dct):
+                self.dct = dct
+            def __str__(self):
+                del self.dct[self]


It would be better to replace the value rather than delete the key. This wouldn't break conditions for dict iteration.

Replacing was kind of weird - I think because it's undefined behaviour I tried to test in this case and replacing could create an object that "fills" the memory location where the dangling value had been. I had some trouble to make that case fail in a predictable way without the INCREFs. It sometimes threw an AttributeError instead of segfaulting.

The predictability of failure is not required. It enough if the test failed or crashed sometimes.

If your replace the value with other value it can be catched since other value have different repr.

serhiy-storchaka · 2017-03-14T16:55:48Z

Lib/test/test_functools.py

+        with self.assertRaises(TypeError):
+            p()
+
+    def test_manually_adding_non_string_keyword(self):


This is the same name as in the above test! This test hides the above test.

serhiy-storchaka · 2017-03-14T16:56:25Z

Lib/test/test_functools.py

+    def test_manually_adding_non_string_keyword(self):
+        p = self.partial(capture)
+        # Adding a non-string/unicode keyword to partial kwargs
+        p.keywords[1234] = 4321


It would be better to test the value that have different repr and str. For example a string.

serhiy-storchaka · 2017-03-14T16:57:49Z

Modules/_functoolsmodule.c

@@ -253,8 +253,11 @@ partial_repr(partialobject *pto)
    /* Pack keyword arguments */
    assert (PyDict_Check(pto->kw));
    for (i = 0; PyDict_Next(pto->kw, &i, &key, &value);) {
-        Py_SETREF(arglist, PyUnicode_FromFormat("%U, %U=%R", arglist,
+        /* Prevent key.__str__ from deleting key or value during formatting. */


...from deleting value...

serhiy-storchaka · 2017-03-14T16:59:01Z

Lib/test/test_functools.py

+        # value alive (at least long enough).
+        class MutatesYourDict(object):
+            def __str__(self):
+                p[self] = ['sth2']


p.keywords[self]

… key

MSeifert04 · 2017-03-14T17:57:15Z

@the-knights-who-say-ni Steps completed and CLA received.

serhiy-storchaka · 2017-03-14T18:48:04Z

And the last thing is left @MSeifert04. Please add an entry in Misc/NEWS (at the start of the Library section). Don't forget to add "Patch by Michael Seifert."

MSeifert04 · 2017-03-15T06:09:47Z

@serhiy-storchaka Thank you for your helpful comments on the pull request and for merging it! 👍

…words are not strings (#649) (#671)

…words are not strings (#649) (#672)

the-knights-who-say-ni added the CLA not signed label Mar 12, 2017

MSeifert04 added 2 commits March 13, 2017 07:01

bpo-29800: Check if the keys of partial.keywords are unicodes in part…

82e4de5

…ial.__repr__

bpo-29800: Use %S formatting for keys in partial.__repr__

cf9a03e

serhiy-storchaka reviewed Mar 14, 2017

View reviewed changes

serhiy-storchaka requested changes Mar 14, 2017

View reviewed changes

serhiy-storchaka added type-bug An unexpected behavior, bug, or error cherry-pick for 3.5 labels Mar 14, 2017

bpo-29800: Simplified and robustified tests and revert incref'ing the…

286faea

… key

Mariatta added needs backport to 3.5 and removed cherry-pick for 3.5 labels Mar 14, 2017

the-knights-who-say-ni added the CLA signed label Mar 14, 2017

serhiy-storchaka approved these changes Mar 14, 2017

View reviewed changes

serhiy-storchaka changed the title ~~bpo-29800: Check if the keys of partial.keywords are unicodes in partial.__repr__~~ bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings Mar 14, 2017

bpo-29800: Add news entry

31cecca

serhiy-storchaka merged commit 6c3d527 into python:master Mar 15, 2017

MSeifert04 deleted the fix_segfault_partial_repr branch March 15, 2017 06:09

serhiy-storchaka pushed a commit that referenced this pull request Mar 15, 2017

bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.key…

53b2667

…words are not strings (#649) (#671)

serhiy-storchaka pushed a commit that referenced this pull request Mar 15, 2017

bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.key…

0641ada

…words are not strings (#649) (#672)

berkerpeksag removed needs backport to 3.5 labels Mar 23, 2017

Uh oh!

bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings #649

bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings #649

Uh oh!

Conversation

MSeifert04 commented Mar 12, 2017

Uh oh!

the-knights-who-say-ni commented Mar 12, 2017

Uh oh!

mention-bot commented Mar 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Mar 14, 2017

Uh oh!

MSeifert04 commented Mar 15, 2017

Uh oh!

Uh oh!

bpo-29800: Fix crashes in partial.repr if the keys of partial.keywords are not strings #649

bpo-29800: Fix crashes in partial.repr if the keys of partial.keywords are not strings #649

MSeifert04 commented Mar 14, 2017 •

edited

Loading