Skip to content

Commit 25369a8

Browse files
authored
[3.14] gh-144503: Pass sys.argv to forkserver as real argv elements (GH-148194) (#148195)
Avoid embedding the parent's sys.argv into the forkserver -c command string via repr(). When sys.argv is large (e.g. thousands of file paths from a pre-commit hook), the resulting single argument could exceed the OS per-argument length limit (MAX_ARG_STRLEN on Linux, typically 128 KiB), causing posix_spawn to fail and the parent to observe a BrokenPipeError. Instead, append the argv entries as separate command-line arguments after -c; the forkserver child reads them back as sys.argv[1:]. This cannot exceed any limit the parent itself did not already satisfy. Regression introduced by gh-143706 / 298d544. (cherry picked from commit 5e9d90b)
1 parent 8f59d40 commit 25369a8

File tree

4 files changed

+118
-4
lines changed

4 files changed

+118
-4
lines changed

Lib/multiprocessing/forkserver.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,18 +142,25 @@ def ensure_running(self):
142142
self._forkserver_alive_fd = None
143143
self._forkserver_pid = None
144144

145-
cmd = ('from multiprocessing.forkserver import main; ' +
146-
'main(%d, %d, %r, **%r)')
145+
# gh-144503: sys_argv is passed as real argv elements after the
146+
# ``-c cmd`` rather than repr'd into main_kws so that a large
147+
# parent sys.argv cannot push the single ``-c`` command string
148+
# over the OS per-argument length limit (MAX_ARG_STRLEN on Linux).
149+
# The child sees them as sys.argv[1:].
150+
cmd = ('import sys; '
151+
'from multiprocessing.forkserver import main; '
152+
'main(%d, %d, %r, sys_argv=sys.argv[1:], **%r)')
147153

148154
main_kws = {}
155+
sys_argv = None
149156
if self._preload_modules:
150157
data = spawn.get_preparation_data('ignore')
151158
if 'sys_path' in data:
152159
main_kws['sys_path'] = data['sys_path']
153160
if 'init_main_from_path' in data:
154161
main_kws['main_path'] = data['init_main_from_path']
155162
if 'sys_argv' in data:
156-
main_kws['sys_argv'] = data['sys_argv']
163+
sys_argv = data['sys_argv']
157164

158165
with socket.socket(socket.AF_UNIX) as listener:
159166
address = connection.arbitrary_address('AF_UNIX')
@@ -175,6 +182,8 @@ def ensure_running(self):
175182
exe = spawn.get_executable()
176183
args = [exe] + util._args_from_interpreter_flags()
177184
args += ['-c', cmd]
185+
if sys_argv is not None:
186+
args += sys_argv
178187
pid = util.spawnv_passfds(exe, args, fds_to_pass)
179188
except:
180189
os.close(alive_w)

Lib/test/_test_multiprocessing.py

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,10 +206,38 @@ def spawn_check_wrapper(*args, **kwargs):
206206
return decorator
207207

208208

209+
def only_run_in_forkserver_testsuite(reason):
210+
"""Returns a decorator: raises SkipTest unless fork is supported
211+
and the current start method is forkserver.
212+
213+
Combines @support.requires_fork() with the single-run semantics of
214+
only_run_in_spawn_testsuite(), but uses the forkserver testsuite as
215+
the single-run target. Appropriate for tests that exercise
216+
os.fork() directly (raw fork or mp.set_start_method("fork") in a
217+
subprocess) and don't vary by start method, since forkserver is
218+
only available on platforms that support fork.
219+
"""
220+
221+
def decorator(test_item):
222+
223+
@functools.wraps(test_item)
224+
def forkserver_check_wrapper(*args, **kwargs):
225+
if not support.has_fork_support:
226+
raise unittest.SkipTest("requires working os.fork()")
227+
if (start_method := multiprocessing.get_start_method()) != "forkserver":
228+
raise unittest.SkipTest(
229+
f"{start_method=}, not 'forkserver'; {reason}")
230+
return test_item(*args, **kwargs)
231+
232+
return forkserver_check_wrapper
233+
234+
return decorator
235+
236+
209237
class TestInternalDecorators(unittest.TestCase):
210238
"""Logic within a test suite that could errantly skip tests? Test it!"""
211239

212-
@unittest.skipIf(sys.platform == "win32", "test requires that fork exists.")
240+
@support.requires_fork()
213241
def test_only_run_in_spawn_testsuite(self):
214242
if multiprocessing.get_start_method() != "spawn":
215243
raise unittest.SkipTest("only run in test_multiprocessing_spawn.")
@@ -233,6 +261,30 @@ def return_four_if_spawn():
233261
finally:
234262
multiprocessing.set_start_method(orig_start_method, force=True)
235263

264+
@support.requires_fork()
265+
def test_only_run_in_forkserver_testsuite(self):
266+
if multiprocessing.get_start_method() != "forkserver":
267+
raise unittest.SkipTest("only run in test_multiprocessing_forkserver.")
268+
269+
try:
270+
@only_run_in_forkserver_testsuite("testing this decorator")
271+
def return_four_if_forkserver():
272+
return 4
273+
except Exception as err:
274+
self.fail(f"expected decorated `def` not to raise; caught {err}")
275+
276+
orig_start_method = multiprocessing.get_start_method(allow_none=True)
277+
try:
278+
multiprocessing.set_start_method("forkserver", force=True)
279+
self.assertEqual(return_four_if_forkserver(), 4)
280+
multiprocessing.set_start_method("spawn", force=True)
281+
with self.assertRaises(unittest.SkipTest) as ctx:
282+
return_four_if_forkserver()
283+
self.assertIn("testing this decorator", str(ctx.exception))
284+
self.assertIn("start_method=", str(ctx.exception))
285+
finally:
286+
multiprocessing.set_start_method(orig_start_method, force=True)
287+
236288

237289
#
238290
# Creates a wrapper for a function which records the time it takes to finish
@@ -6973,6 +7025,23 @@ def test_preload_main_sys_argv(self):
69737025
'',
69747026
])
69757027

7028+
@only_run_in_forkserver_testsuite("forkserver specific test.")
7029+
def test_preload_main_large_sys_argv(self):
7030+
# gh-144503: a very large parent sys.argv must not prevent the
7031+
# forkserver from starting (it previously overflowed the OS
7032+
# per-argument length limit when repr'd into the -c command string).
7033+
name = os.path.join(os.path.dirname(__file__),
7034+
'mp_preload_large_sysargv.py')
7035+
_, out, err = test.support.script_helper.assert_python_ok(name)
7036+
self.assertEqual(err, b'')
7037+
7038+
out = out.decode().split("\n")
7039+
self.assertEqual(out, [
7040+
'preload:5002:sentinel',
7041+
'worker:5002:sentinel',
7042+
'',
7043+
])
7044+
69767045
#
69777046
# Mixins
69787047
#
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# gh-144503: Test that the forkserver can start when the parent process has
2+
# a very large sys.argv. Prior to the fix, sys.argv was repr'd into the
3+
# forkserver ``-c`` command string which could exceed the OS limit on the
4+
# length of a single argv element (MAX_ARG_STRLEN on Linux, ~128 KiB),
5+
# causing posix_spawn to fail and the parent to see a BrokenPipeError.
6+
7+
import multiprocessing
8+
import sys
9+
10+
EXPECTED_LEN = 5002 # argv[0] + 5000 padding entries + sentinel
11+
12+
13+
def fun():
14+
print(f"worker:{len(sys.argv)}:{sys.argv[-1]}")
15+
16+
17+
if __name__ == "__main__":
18+
# Inflate sys.argv well past 128 KiB before the forkserver is started.
19+
sys.argv[1:] = ["x" * 50] * 5000 + ["sentinel"]
20+
assert len(sys.argv) == EXPECTED_LEN
21+
22+
ctx = multiprocessing.get_context("forkserver")
23+
p = ctx.Process(target=fun)
24+
p.start()
25+
p.join()
26+
sys.exit(p.exitcode)
27+
else:
28+
# This branch runs when the forkserver preloads this module as
29+
# __mp_main__; confirm the large argv was propagated intact.
30+
print(f"preload:{len(sys.argv)}:{sys.argv[-1]}")
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Fix a regression introduced in 3.14.3 and 3.13.12 where the
2+
:mod:`multiprocessing` ``forkserver`` start method would fail with
3+
:exc:`BrokenPipeError` when the parent process had a very large
4+
:data:`sys.argv`. The argv is now passed to the forkserver as separate
5+
command-line arguments rather than being embedded in the ``-c`` command
6+
string, avoiding the operating system's per-argument length limit.

0 commit comments

Comments
 (0)