From c0f43606f8c82cc9511c51fa2a9d12d058fbb438 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 23 Sep 2023 13:09:04 +0300 Subject: [PATCH 1/6] gh-109786: Fix leaks when re-enter itertools.pairwise.__next__() --- Lib/test/test_itertools.py | 32 +++++++++++++++++++ ...-09-23-14-40-51.gh-issue-109786.UX3pKv.rst | 2 ++ Modules/itertoolsmodule.c | 10 ++++-- 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 512745e45350d1..8c2b9260350d58 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -1152,6 +1152,38 @@ def test_pairwise(self): with self.assertRaises(TypeError): pairwise(None) # non-iterable argument + def test_pairwise_reenter(self): + def check(reenter_at, expected): + class I: + count = 0 + def __iter__(self): + return self + def __next__(self): + self.count +=1 + if self.count == reenter_at: + return next(it) + return [self.count] # new object + + it = pairwise(I()) + for item in expected: + self.assertEqual(next(it), item) + + check(1, [ + (([2], [3]), [4]), + ([4], [5]), + ]) + check(2, [ + ([1], ([3], [4])), + (([3], [4]), [5]), + ([5], [6]), + ]) + check(3, [ + ([1], [2]), + ([2], ([2], [4])), + (([2], [4]), [5]), + ([5], [6]), + ]) + def test_product(self): for args, result in [ ([], [()]), # zero iterables diff --git a/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst b/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst new file mode 100644 index 00000000000000..fdd4a5534e4906 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst @@ -0,0 +1,2 @@ +Fix reference leaks when re-enter the ``__next__()`` method of +:class:`itertools.pairwise`. diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 4ed34aa5bde827..ddb8703dd3bf80 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -330,21 +330,27 @@ pairwise_next(pairwiseobject *po) return NULL; } if (old == NULL) { - po->old = old = (*Py_TYPE(it)->tp_iternext)(it); + old = (*Py_TYPE(it)->tp_iternext)(it); if (old == NULL) { Py_CLEAR(po->it); + Py_CLEAR(po->old); return NULL; } } + else { + Py_INCREF(old); + } new = (*Py_TYPE(it)->tp_iternext)(it); if (new == NULL) { Py_CLEAR(po->it); Py_CLEAR(po->old); + Py_DECREF(old); return NULL; } /* Future optimization: Reuse the result tuple as we do in enumerate() */ result = PyTuple_Pack(2, old, new); - Py_SETREF(po->old, new); + Py_XSETREF(po->old, new); + Py_DECREF(old); return result; } From 6c6556c59fbbd72ae1ad22454c24cc14c83d64ea Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 23 Sep 2023 15:05:29 +0300 Subject: [PATCH 2/6] More closely reproduce current results. --- Lib/test/test_itertools.py | 6 +++--- Modules/itertoolsmodule.c | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 8c2b9260350d58..22a732a1a50f20 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -1173,9 +1173,9 @@ def __next__(self): ([4], [5]), ]) check(2, [ - ([1], ([3], [4])), - (([3], [4]), [5]), - ([5], [6]), + ([1], ([1], [3])), + (([1], [3]), [4]), + ([4], [5]), ]) check(3, [ ([1], [2]), diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index ddb8703dd3bf80..c3ac2b981d5364 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -336,6 +336,10 @@ pairwise_next(pairwiseobject *po) Py_CLEAR(po->old); return NULL; } + if (po->old == NULL) { + po->old = old; + Py_INCREF(old); + } } else { Py_INCREF(old); From 5ffe65a3eeb1d6351c4bbf7e6a7f47b4bfe05e9c Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 23 Sep 2023 15:46:02 +0300 Subject: [PATCH 3/6] Add tests for multiple re-entrancy points. --- Lib/test/test_itertools.py | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 22a732a1a50f20..0e12d93fef95a9 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -1160,7 +1160,7 @@ def __iter__(self): return self def __next__(self): self.count +=1 - if self.count == reenter_at: + if self.count in reenter_at: return next(it) return [self.count] # new object @@ -1168,21 +1168,39 @@ def __next__(self): for item in expected: self.assertEqual(next(it), item) - check(1, [ + check({1}, [ (([2], [3]), [4]), ([4], [5]), ]) - check(2, [ + check({2}, [ ([1], ([1], [3])), (([1], [3]), [4]), ([4], [5]), ]) - check(3, [ + check({3}, [ ([1], [2]), ([2], ([2], [4])), (([2], [4]), [5]), ([5], [6]), ]) + check({1, 2}, [ + ((([3], [4]), [5]), [6]), + ([6], [7]), + ]) + check({1, 3}, [ + (([2], ([2], [4])), [5]), + ([5], [6]), + ]) + check({1, 4}, [ + (([2], [3]), ([3], [5])), + (([3], [5]), [6]), + ([6], [7]), + ]) + check({2, 3}, [ + ([1], ([1], ([1], [4]))), + (([1], ([1], [4])), [5]), + ([5], [6]), + ]) def test_product(self): for args, result in [ From 4c4479129ada05fed9b426a5cd653c7c4034b4f2 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 24 Sep 2023 09:20:10 +0300 Subject: [PATCH 4/6] Yet more closely reproduce current results. --- Lib/test/test_itertools.py | 4 ++-- Modules/itertoolsmodule.c | 10 ++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 0e12d93fef95a9..a71561e97bedf4 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -1192,8 +1192,8 @@ def __next__(self): ([5], [6]), ]) check({1, 4}, [ - (([2], [3]), ([3], [5])), - (([3], [5]), [6]), + (([2], [3]), (([2], [3]), [5])), + ((([2], [3]), [5]), [6]), ([6], [7]), ]) check({2, 3}, [ diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index c3ac2b981d5364..3e94c52eb392d7 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -331,19 +331,13 @@ pairwise_next(pairwiseobject *po) } if (old == NULL) { old = (*Py_TYPE(it)->tp_iternext)(it); + Py_XSETREF(po->old, old); if (old == NULL) { Py_CLEAR(po->it); - Py_CLEAR(po->old); return NULL; } - if (po->old == NULL) { - po->old = old; - Py_INCREF(old); - } - } - else { - Py_INCREF(old); } + Py_INCREF(old); new = (*Py_TYPE(it)->tp_iternext)(it); if (new == NULL) { Py_CLEAR(po->it); From b047b2a79033baba5b6ec417d5cca5e36ebcf4d7 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 24 Sep 2023 10:41:07 +0300 Subject: [PATCH 5/6] Fix a crash due to `it` holding borrowed reference. --- Lib/test/test_itertools.py | 22 ++++++++++++++++++++++ Modules/itertoolsmodule.c | 5 +++++ 2 files changed, 27 insertions(+) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index a71561e97bedf4..705e880d98685e 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -1202,6 +1202,28 @@ def __next__(self): ([5], [6]), ]) + def test_pairwise_reenter2(self): + def check(maxcount, expected): + class I: + count = 0 + def __iter__(self): + return self + def __next__(self): + if self.count >= maxcount: + raise StopIteration + self.count +=1 + if self.count == 1: + return next(it, None) + return [self.count] # new object + + it = pairwise(I()) + self.assertEqual(list(it), expected) + + check(1, []) + check(2, []) + check(3, []) + check(4, [(([2], [3]), [4])]) + def test_product(self): for args, result in [ ([], [()]), # zero iterables diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 3e94c52eb392d7..ab99fa4d873bf5 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -336,6 +336,11 @@ pairwise_next(pairwiseobject *po) Py_CLEAR(po->it); return NULL; } + it = po->it; + if (it == NULL) { + Py_CLEAR(po->old); + return NULL; + } } Py_INCREF(old); new = (*Py_TYPE(it)->tp_iternext)(it); From 37bc689f8915087dcd33cd4a891c359f587909c8 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 4 Dec 2023 13:23:11 +0200 Subject: [PATCH 6/6] Update 2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst --- .../next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst b/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst index fdd4a5534e4906..07222fa339d703 100644 --- a/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst +++ b/Misc/NEWS.d/next/Library/2023-09-23-14-40-51.gh-issue-109786.UX3pKv.rst @@ -1,2 +1,2 @@ -Fix reference leaks when re-enter the ``__next__()`` method of +Fix possible reference leaks and crash when re-enter the ``__next__()`` method of :class:`itertools.pairwise`.