Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commite1aa52c

Browse files
picnixzmiss-islington
authored andcommitted
pythongh-126033: fix UAF inxml.etree.ElementTree.Element.remove when concurrent mutations happen (pythonGH-126124)
(cherry picked from commitbab1398)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
1 parent525eddf commite1aa52c

File tree

3 files changed

+213
-36
lines changed

3 files changed

+213
-36
lines changed

‎Lib/test/test_xml_etree.py‎

Lines changed: 178 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,11 @@
1818
importtextwrap
1919
importtypes
2020
importunittest
21+
importunittest.mockasmock
2122
importwarnings
2223
importweakref
2324

25+
fromcontextlibimportnullcontext
2426
fromfunctoolsimportpartial
2527
fromitertoolsimportproduct,islice
2628
fromtestimportsupport
@@ -121,6 +123,21 @@
121123
</foo>
122124
"""
123125

126+
defis_python_implementation():
127+
assertETisnotNone,"ET must be initialized"
128+
assertpyETisnotNone,"pyET must be initialized"
129+
returnETispyET
130+
131+
132+
defequal_wrapper(cls):
133+
"""Mock cls.__eq__ to check whether it has been called or not.
134+
135+
The behaviour of cls.__eq__ (side-effects included) is left as is.
136+
"""
137+
eq=cls.__eq__
138+
returnmock.patch.object(cls,"__eq__",autospec=True,wraps=eq)
139+
140+
124141
defcheckwarnings(*filters,quiet=False):
125142
defdecorator(test):
126143
defnewtest(*args,**kwargs):
@@ -2562,6 +2579,7 @@ def test_pickle_issue18997(self):
25622579

25632580

25642581
classBadElementTest(ElementTestCase,unittest.TestCase):
2582+
25652583
deftest_extend_mutable_list(self):
25662584
classX:
25672585
@property
@@ -2600,18 +2618,168 @@ class Y(X, ET.Element):
26002618
e=ET.Element('foo')
26012619
e.extend(L)
26022620

2603-
deftest_remove_with_mutating(self):
2604-
classX(ET.Element):
2621+
deftest_remove_with_clear_assume_missing(self):
2622+
# gh-126033: Check that a concurrent clear() for an assumed-to-be
2623+
# missing element does not make the interpreter crash.
2624+
self.do_test_remove_with_clear(raises=True)
2625+
2626+
deftest_remove_with_clear_assume_existing(self):
2627+
# gh-126033: Check that a concurrent clear() for an assumed-to-be
2628+
# existing element does not make the interpreter crash.
2629+
self.do_test_remove_with_clear(raises=False)
2630+
2631+
defdo_test_remove_with_clear(self,*,raises):
2632+
2633+
# Until the discrepency between "del root[:]" and "root.clear()" is
2634+
# resolved, we need to keep two tests. Previously, using "del root[:]"
2635+
# did not crash with the reproducer of gh-126033 while "root.clear()"
2636+
# did.
2637+
2638+
classE(ET.Element):
2639+
"""Local class to be able to mock E.__eq__ for introspection."""
2640+
2641+
classX(E):
26052642
def__eq__(self,o):
2606-
dele[:]
2607-
returnFalse
2608-
e=ET.Element('foo')
2609-
e.extend([X('bar')])
2610-
self.assertRaises(ValueError,e.remove,ET.Element('baz'))
2643+
delroot[:]
2644+
returnnotraises
26112645

2612-
e=ET.Element('foo')
2613-
e.extend([ET.Element('bar')])
2614-
self.assertRaises(ValueError,e.remove,X('baz'))
2646+
classY(E):
2647+
def__eq__(self,o):
2648+
root.clear()
2649+
returnnotraises
2650+
2651+
ifraises:
2652+
get_checker_context=lambda:self.assertRaises(ValueError)
2653+
else:
2654+
get_checker_context=nullcontext
2655+
2656+
self.assertIs(E.__eq__,object.__eq__)
2657+
2658+
forZ,side_effectin [(X,'del root[:]'), (Y,'root.clear()')]:
2659+
self.enterContext(self.subTest(side_effect=side_effect))
2660+
2661+
# test removing R() from [U()]
2662+
forR,U,descriptionin [
2663+
(E,Z,"remove missing E() from [Z()]"),
2664+
(Z,E,"remove missing Z() from [E()]"),
2665+
(Z,Z,"remove missing Z() from [Z()]"),
2666+
]:
2667+
withself.subTest(description):
2668+
root=E('top')
2669+
root.extend([U('one')])
2670+
withget_checker_context():
2671+
root.remove(R('missing'))
2672+
2673+
# test removing R() from [U(), V()]
2674+
cases=self.cases_for_remove_missing_with_mutations(E,Z)
2675+
forR,U,V,descriptionincases:
2676+
withself.subTest(description):
2677+
root=E('top')
2678+
root.extend([U('one'),V('two')])
2679+
withget_checker_context():
2680+
root.remove(R('missing'))
2681+
2682+
# Test removing root[0] from [Z()].
2683+
#
2684+
# Since we call root.remove() with root[0], Z.__eq__()
2685+
# will not be called (we branch on the fast Py_EQ path).
2686+
withself.subTest("remove root[0] from [Z()]"):
2687+
root=E('top')
2688+
root.append(Z('rem'))
2689+
withequal_wrapper(E)asf,equal_wrapper(Z)asg:
2690+
root.remove(root[0])
2691+
f.assert_not_called()
2692+
g.assert_not_called()
2693+
2694+
# Test removing root[1] (of type R) from [U(), R()].
2695+
is_special=is_python_implementation()andraisesandZisY
2696+
ifis_python_implementation()andraisesandZisY:
2697+
# In pure Python, using root.clear() sets the children
2698+
# list to [] without calling list.clear().
2699+
#
2700+
# For this reason, the call to root.remove() first
2701+
# checks root[0] and sets the children list to []
2702+
# since either root[0] or root[1] is an evil element.
2703+
#
2704+
# Since checking root[1] still uses the old reference
2705+
# to the children list, PyObject_RichCompareBool() branches
2706+
# to the fast Py_EQ path and Y.__eq__() is called exactly
2707+
# once (when checking root[0]).
2708+
continue
2709+
else:
2710+
cases=self.cases_for_remove_existing_with_mutations(E,Z)
2711+
forR,U,descriptionincases:
2712+
withself.subTest(description):
2713+
root=E('top')
2714+
root.extend([U('one'),R('rem')])
2715+
withget_checker_context():
2716+
root.remove(root[1])
2717+
2718+
deftest_remove_with_mutate_root_assume_missing(self):
2719+
# gh-126033: Check that a concurrent mutation for an assumed-to-be
2720+
# missing element does not make the interpreter crash.
2721+
self.do_test_remove_with_mutate_root(raises=True)
2722+
2723+
deftest_remove_with_mutate_root_assume_existing(self):
2724+
# gh-126033: Check that a concurrent mutation for an assumed-to-be
2725+
# existing element does not make the interpreter crash.
2726+
self.do_test_remove_with_mutate_root(raises=False)
2727+
2728+
defdo_test_remove_with_mutate_root(self,*,raises):
2729+
E=ET.Element
2730+
2731+
classZ(E):
2732+
def__eq__(self,o):
2733+
delroot[0]
2734+
returnnotraises
2735+
2736+
ifraises:
2737+
get_checker_context=lambda:self.assertRaises(ValueError)
2738+
else:
2739+
get_checker_context=nullcontext
2740+
2741+
# test removing R() from [U(), V()]
2742+
cases=self.cases_for_remove_missing_with_mutations(E,Z)
2743+
forR,U,V,descriptionincases:
2744+
withself.subTest(description):
2745+
root=E('top')
2746+
root.extend([U('one'),V('two')])
2747+
withget_checker_context():
2748+
root.remove(R('missing'))
2749+
2750+
# test removing root[1] (of type R) from [U(), R()]
2751+
cases=self.cases_for_remove_existing_with_mutations(E,Z)
2752+
forR,U,descriptionincases:
2753+
withself.subTest(description):
2754+
root=E('top')
2755+
root.extend([U('one'),R('rem')])
2756+
withget_checker_context():
2757+
root.remove(root[1])
2758+
2759+
defcases_for_remove_missing_with_mutations(self,E,Z):
2760+
# Cases for removing R() from [U(), V()].
2761+
# The case U = V = R = E is not interesting as there is no mutation.
2762+
forU,Vin [(E,Z), (Z,E), (Z,Z)]:
2763+
description= (f"remove missing{E.__name__}() from "
2764+
f"[{U.__name__}(),{V.__name__}()]")
2765+
yieldE,U,V,description
2766+
2767+
forU,Vin [(E,E), (E,Z), (Z,E), (Z,Z)]:
2768+
description= (f"remove missing{Z.__name__}() from "
2769+
f"[{U.__name__}(),{V.__name__}()]")
2770+
yieldZ,U,V,description
2771+
2772+
defcases_for_remove_existing_with_mutations(self,E,Z):
2773+
# Cases for removing root[1] (of type R) from [U(), R()].
2774+
# The case U = R = E is not interesting as there is no mutation.
2775+
forU,R,descriptionin [
2776+
(E,Z,"remove root[1] from [E(), Z()]"),
2777+
(Z,E,"remove root[1] from [Z(), E()]"),
2778+
(Z,Z,"remove root[1] from [Z(), Z()]"),
2779+
]:
2780+
description= (f"remove root[1] (of type{R.__name__}) "
2781+
f"from [{U.__name__}(),{R.__name__}()]")
2782+
yieldR,U,description
26152783

26162784
@support.infinite_recursion(25)
26172785
deftest_recursive_repr(self):
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
:mod:`xml.etree.ElementTree`: Fix a crash in:meth:`Element.remove
2+
<xml.etree.ElementTree.Element.remove>` when the element is
3+
concurrently mutated. Patch by Bénédikt Tran.

‎Modules/_elementtree.c‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -845,6 +845,7 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
845845
if (element_resize(element,self->extra->length)<0)
846846
gotoerror;
847847

848+
// TODO(picnixz): check for an evil child's __deepcopy__ on 'self'
848849
for (i=0;i<self->extra->length;i++) {
849850
PyObject*child=deepcopy(st,self->extra->children[i],memo);
850851
if (!child|| !Element_Check(st,child)) {
@@ -1635,42 +1636,47 @@ _elementtree_Element_remove_impl(ElementObject *self, PyObject *subelement)
16351636
/*[clinic end generated code: output=38fe6c07d6d87d1f input=6133e1d05597d5ee]*/
16361637
{
16371638
Py_ssize_ti;
1638-
intrc;
1639-
PyObject*found;
1640-
1641-
if (!self->extra) {
1642-
/* element has no children, so raise exception */
1643-
PyErr_SetString(
1644-
PyExc_ValueError,
1645-
"list.remove(x): x not in list"
1646-
);
1647-
returnNULL;
1648-
}
1649-
1650-
for (i=0;i<self->extra->length;i++) {
1651-
if (self->extra->children[i]==subelement)
1639+
// When iterating over the list of children, we need to check that the
1640+
// list is not cleared (self->extra != NULL) and that we are still within
1641+
// the correct bounds (i < self->extra->length).
1642+
//
1643+
// We deliberately avoid protecting against children lists that grow
1644+
// faster than the index since list objects do not protect against it.
1645+
intrc=0;
1646+
for (i=0;self->extra&&i<self->extra->length;i++) {
1647+
if (self->extra->children[i]==subelement) {
1648+
rc=1;
16521649
break;
1653-
rc=PyObject_RichCompareBool(self->extra->children[i],subelement,Py_EQ);
1654-
if (rc>0)
1655-
break;
1656-
if (rc<0)
1650+
}
1651+
PyObject*child=Py_NewRef(self->extra->children[i]);
1652+
rc=PyObject_RichCompareBool(child,subelement,Py_EQ);
1653+
Py_DECREF(child);
1654+
if (rc<0) {
16571655
returnNULL;
1656+
}
1657+
elseif (rc>0) {
1658+
break;
1659+
}
16581660
}
16591661

1660-
if (i >=self->extra->length) {
1661-
/* subelement is not in children, so raise exception */
1662-
PyErr_SetString(
1663-
PyExc_ValueError,
1664-
"list.remove(x): x not in list"
1665-
);
1662+
if (rc==0) {
1663+
PyErr_SetString(PyExc_ValueError,"list.remove(x): x not in list");
16661664
returnNULL;
16671665
}
16681666

1669-
found=self->extra->children[i];
1667+
// An extra check must be done if the mutation occurs at the very last
1668+
// step and removes or clears the 'extra' list (the condition on the
1669+
// length would not be satisfied any more).
1670+
if (self->extra==NULL||i >=self->extra->length) {
1671+
Py_RETURN_NONE;
1672+
}
1673+
1674+
PyObject*found=self->extra->children[i];
16701675

16711676
self->extra->length--;
1672-
for (;i<self->extra->length;i++)
1677+
for (;i<self->extra->length;i++) {
16731678
self->extra->children[i]=self->extra->children[i+1];
1679+
}
16741680

16751681
Py_DECREF(found);
16761682
Py_RETURN_NONE;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp