Skip to content

Commit bb4af8a

Browse files
committed
gh-42400: Fix buffer overflow in _Py_wrealpath for long paths
2 parents 3255a3d + a486d45 commit bb4af8a

6 files changed

Lines changed: 123 additions & 67 deletions

File tree

Doc/library/xml.etree.elementtree.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,10 @@ Functions
656656
.. versionchanged:: 3.13
657657
Added the :meth:`!close` method.
658658

659+
.. versionchanged:: next
660+
A :exc:`ResourceWarning` is now emitted if the iterator opened a file
661+
and is not explicitly closed.
662+
659663

660664
.. function:: parse(source, parser=None)
661665

Doc/whatsnew/3.15.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1244,3 +1244,9 @@ that may require changes to your code.
12441244

12451245
* :meth:`~mmap.mmap.resize` has been removed on platforms that don't support the
12461246
underlying syscall, instead of raising a :exc:`SystemError`.
1247+
1248+
* Resource warning is now emitted for unclosed
1249+
:func:`xml.etree.ElementTree.iterparse` iterator if it opened a file.
1250+
Use its :meth:`!close` method or the :func:`contextlib.closing` context
1251+
manager to close it.
1252+
(Contributed by Osama Abdelkader and Serhiy Storchaka in :gh:`140601`.)

Lib/test/test_fileutils.py

Lines changed: 54 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
1-
# Run tests for functions in Python/fileutils.c.
2-
31
import os
42
import os.path
53
import unittest
64
import tempfile
75
import shutil
8-
from test.support import import_helper
6+
from test.support import import_helper, os_helper
97

108
# Skip this test if the _testcapi module isn't available.
119
_testcapi = import_helper.import_module('_testinternalcapi')
@@ -29,86 +27,79 @@ def test_capi_normalize_path(self):
2927

3028

3129
class RealpathTests(unittest.TestCase):
32-
"""Tests for _Py_wrealpath used by os.path.realpath"""
33-
34-
def test_realpath_long_path(self):
35-
"""Test that realpath handles paths longer than MAXPATHLEN (4096)"""
36-
if os.name == 'nt':
37-
raise unittest.SkipTest('POSIX-specific test')
30+
"""Tests for _Py_wrealpath used by os.path.realpath."""
3831

39-
base = tempfile.mkdtemp()
40-
original_cwd = os.getcwd()
41-
try:
42-
os.chdir(base)
32+
def setUp(self):
33+
self.base = None
4334

44-
for i in range(85):
45-
dirname = f"d{i:03d}_" + "x" * 44
46-
os.mkdir(dirname)
47-
os.chdir(dirname)
35+
def tearDown(self):
36+
if self.base and os.path.exists(self.base):
37+
shutil.rmtree(self.base, ignore_errors=True)
4838

49-
full_path = os.getcwd()
39+
def test_realpath_long_path(self):
40+
"""Test that realpath handles paths longer than MAXPATHLEN."""
41+
if os.name == 'nt':
42+
self.skipTest('POSIX-specific test')
5043

51-
self.assertGreater(len(full_path), 4096,
52-
f"Path should exceed MAXPATHLEN, got {len(full_path)}")
44+
self.base = tempfile.mkdtemp()
45+
current_path = self.base
5346

54-
# Main test: realpath should not crash on long paths
55-
result = os.path.realpath(full_path)
47+
for i in range(25):
48+
dirname = f"d{i:03d}_" + "x" * 195
49+
current_path = os.path.join(current_path, dirname)
50+
try:
51+
os.mkdir(current_path)
52+
except OSError as e:
53+
self.skipTest(f"Cannot create long paths on this platform: {e}")
5654

57-
self.assertTrue(os.path.isabs(result))
58-
self.assertGreater(len(result), 4096)
59-
# Note: os.path.exists() may fail on very long paths
60-
# The important thing is realpath() doesn't crash
55+
full_path = os.path.abspath(current_path)
56+
if len(full_path) <= 4096:
57+
self.skipTest(f"Path not long enough ({len(full_path)} bytes)")
6158

62-
finally:
63-
os.chdir(original_cwd)
64-
shutil.rmtree(base, ignore_errors=True)
59+
result = os.path.realpath(full_path)
60+
self.assertTrue(os.path.isabs(result))
61+
self.assertGreater(len(result), 4096)
6562

6663
def test_realpath_nonexistent_with_strict(self):
67-
"""Test that realpath with strict=True raises for nonexistent paths"""
64+
"""Test that realpath with strict=True raises for nonexistent paths."""
6865
if os.name == 'nt':
69-
raise unittest.SkipTest('POSIX-specific test')
70-
71-
base = tempfile.mkdtemp()
72-
try:
73-
nonexistent = os.path.join(base, "does_not_exist", "subdir")
66+
self.skipTest('POSIX-specific test')
7467

75-
# Without strict, should return the path
76-
result = os.path.realpath(nonexistent, strict=False)
77-
self.assertIsNotNone(result)
68+
self.base = tempfile.mkdtemp()
69+
nonexistent = os.path.join(self.base, "does_not_exist", "subdir")
7870

79-
# With strict=True, should raise an error
80-
with self.assertRaises(OSError):
81-
os.path.realpath(nonexistent, strict=True)
71+
result = os.path.realpath(nonexistent, strict=False)
72+
self.assertIsNotNone(result)
8273

83-
finally:
84-
shutil.rmtree(base, ignore_errors=True)
74+
with self.assertRaises(OSError):
75+
os.path.realpath(nonexistent, strict=True)
8576

77+
@os_helper.skip_unless_symlink
8678
def test_realpath_symlink_long_path(self):
87-
"""Test realpath with symlinks in long paths"""
79+
"""Test realpath with symlinks in long paths."""
8880
if os.name == 'nt':
89-
raise unittest.SkipTest('POSIX-specific test')
81+
self.skipTest('POSIX-specific test')
9082

91-
base = tempfile.mkdtemp()
92-
try:
93-
# Create a long path
94-
current = base
95-
for i in range(30):
96-
dirname = f"d{i:03d}_" + "x" * 44
97-
current = os.path.join(current, dirname)
98-
os.mkdir(current)
99-
100-
# Create a symlink pointing to the long path
101-
symlink = os.path.join(base, "link")
102-
os.symlink(current, symlink)
83+
self.base = tempfile.mkdtemp()
84+
current_path = self.base
10385

104-
# Resolve the symlink
105-
result = os.path.realpath(symlink)
86+
for i in range(15):
87+
dirname = f"d{i:03d}_" + "x" * 195
88+
current_path = os.path.join(current_path, dirname)
89+
try:
90+
os.mkdir(current_path)
91+
except OSError as e:
92+
self.skipTest(f"Cannot create long paths on this platform: {e}")
10693

107-
self.assertEqual(os.path.normpath(result), os.path.normpath(current))
108-
self.assertGreater(len(result), 1500)
94+
symlink = os.path.join(self.base, "link")
95+
try:
96+
os.symlink(current_path, symlink)
97+
except (OSError, NotImplementedError) as e:
98+
self.skipTest(f"Cannot create symlinks on this platform: {e}")
10999

110-
finally:
111-
shutil.rmtree(base, ignore_errors=True)
100+
result = os.path.realpath(symlink)
101+
self.assertEqual(os.path.normpath(result), os.path.normpath(current_path))
102+
self.assertGreater(len(result), 1500)
112103

113104

114105
if __name__ == "__main__":

Lib/test/test_xml_etree.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1436,18 +1436,40 @@ def test_nonexistent_file(self):
14361436

14371437
def test_resource_warnings_not_exhausted(self):
14381438
# Not exhausting the iterator still closes the underlying file (bpo-43292)
1439+
# Not closing before del should emit ResourceWarning
14391440
it = ET.iterparse(SIMPLE_XMLFILE)
14401441
with warnings_helper.check_no_resource_warning(self):
1442+
it.close()
1443+
del it
1444+
gc_collect()
1445+
1446+
it = ET.iterparse(SIMPLE_XMLFILE)
1447+
with self.assertWarns(ResourceWarning) as wm:
14411448
del it
14421449
gc_collect()
1450+
# Not 'unclosed file'.
1451+
self.assertIn('unclosed iterparse iterator', str(wm.warning))
1452+
self.assertIn(repr(SIMPLE_XMLFILE), str(wm.warning))
1453+
self.assertEqual(wm.filename, __file__)
14431454

14441455
it = ET.iterparse(SIMPLE_XMLFILE)
14451456
with warnings_helper.check_no_resource_warning(self):
14461457
action, elem = next(it)
1458+
it.close()
14471459
self.assertEqual((action, elem.tag), ('end', 'element'))
14481460
del it, elem
14491461
gc_collect()
14501462

1463+
it = ET.iterparse(SIMPLE_XMLFILE)
1464+
with self.assertWarns(ResourceWarning) as wm:
1465+
action, elem = next(it)
1466+
self.assertEqual((action, elem.tag), ('end', 'element'))
1467+
del it, elem
1468+
gc_collect()
1469+
self.assertIn('unclosed iterparse iterator', str(wm.warning))
1470+
self.assertIn(repr(SIMPLE_XMLFILE), str(wm.warning))
1471+
self.assertEqual(wm.filename, __file__)
1472+
14511473
def test_resource_warnings_failed_iteration(self):
14521474
self.addCleanup(os_helper.unlink, TESTFN)
14531475
with open(TESTFN, "wb") as f:
@@ -1461,15 +1483,40 @@ def test_resource_warnings_failed_iteration(self):
14611483
next(it)
14621484
self.assertEqual(str(cm.exception),
14631485
'junk after document element: line 1, column 12')
1486+
it.close()
14641487
del cm, it
14651488
gc_collect()
14661489

1490+
it = ET.iterparse(TESTFN)
1491+
action, elem = next(it)
1492+
self.assertEqual((action, elem.tag), ('end', 'document'))
1493+
with self.assertWarns(ResourceWarning) as wm:
1494+
with self.assertRaises(ET.ParseError) as cm:
1495+
next(it)
1496+
self.assertEqual(str(cm.exception),
1497+
'junk after document element: line 1, column 12')
1498+
del cm, it
1499+
gc_collect()
1500+
self.assertIn('unclosed iterparse iterator', str(wm.warning))
1501+
self.assertIn(repr(TESTFN), str(wm.warning))
1502+
self.assertEqual(wm.filename, __file__)
1503+
14671504
def test_resource_warnings_exhausted(self):
14681505
it = ET.iterparse(SIMPLE_XMLFILE)
14691506
with warnings_helper.check_no_resource_warning(self):
1507+
list(it)
1508+
it.close()
1509+
del it
1510+
gc_collect()
1511+
1512+
it = ET.iterparse(SIMPLE_XMLFILE)
1513+
with self.assertWarns(ResourceWarning) as wm:
14701514
list(it)
14711515
del it
14721516
gc_collect()
1517+
self.assertIn('unclosed iterparse iterator', str(wm.warning))
1518+
self.assertIn(repr(SIMPLE_XMLFILE), str(wm.warning))
1519+
self.assertEqual(wm.filename, __file__)
14731520

14741521
def test_close_not_exhausted(self):
14751522
iterparse = ET.iterparse

Lib/xml/etree/ElementTree.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,16 +1261,20 @@ def iterator(source):
12611261
gen = iterator(source)
12621262
class IterParseIterator(collections.abc.Iterator):
12631263
__next__ = gen.__next__
1264+
12641265
def close(self):
1266+
nonlocal close_source
12651267
if close_source:
12661268
source.close()
1269+
close_source = False
12671270
gen.close()
12681271

1269-
def __del__(self):
1270-
# TODO: Emit a ResourceWarning if it was not explicitly closed.
1271-
# (When the close() method will be supported in all maintained Python versions.)
1272+
def __del__(self, _warn=warnings.warn):
12721273
if close_source:
1273-
source.close()
1274+
try:
1275+
_warn(f"unclosed iterparse iterator {source.name!r}", ResourceWarning, stacklevel=2)
1276+
finally:
1277+
source.close()
12741278

12751279
it = IterParseIterator()
12761280
it.root = None
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
:func:`xml.etree.ElementTree.iterparse` now emits a :exc:`ResourceWarning`
2+
when the iterator is not explicitly closed and was opened with a filename.
3+
This helps developers identify and fix resource leaks. Patch by Osama
4+
Abdelkader.

0 commit comments

Comments
 (0)