Skip to content

Commit 3038ee9

Browse files
authored
[3.13] gh-144503: Pass sys.argv to forkserver as real argv elements (GH-148194) (#148196)
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 c358b89 commit 3038ee9

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
@@ -124,18 +124,25 @@ def ensure_running(self):
124124
self._forkserver_alive_fd = None
125125
self._forkserver_pid = None
126126

127-
cmd = ('from multiprocessing.forkserver import main; ' +
128-
'main(%d, %d, %r, **%r)')
127+
# gh-144503: sys_argv is passed as real argv elements after the
128+
# ``-c cmd`` rather than repr'd into main_kws so that a large
129+
# parent sys.argv cannot push the single ``-c`` command string
130+
# over the OS per-argument length limit (MAX_ARG_STRLEN on Linux).
131+
# The child sees them as sys.argv[1:].
132+
cmd = ('import sys; '
133+
'from multiprocessing.forkserver import main; '
134+
'main(%d, %d, %r, sys_argv=sys.argv[1:], **%r)')
129135

130136
main_kws = {}
137+
sys_argv = None
131138
if self._preload_modules:
132139
data = spawn.get_preparation_data('ignore')
133140
if 'sys_path' in data:
134141
main_kws['sys_path'] = data['sys_path']
135142
if 'init_main_from_path' in data:
136143
main_kws['main_path'] = data['init_main_from_path']
137144
if 'sys_argv' in data:
138-
main_kws['sys_argv'] = data['sys_argv']
145+
sys_argv = data['sys_argv']
139146

140147
with socket.socket(socket.AF_UNIX) as listener:
141148
address = connection.arbitrary_address('AF_UNIX')
@@ -154,6 +161,8 @@ def ensure_running(self):
154161
exe = spawn.get_executable()
155162
args = [exe] + util._args_from_interpreter_flags()
156163
args += ['-c', cmd]
164+
if sys_argv is not None:
165+
args += sys_argv
157166
pid = util.spawnv_passfds(exe, args, fds_to_pass)
158167
except:
159168
os.close(alive_w)

Lib/test/_test_multiprocessing.py

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

207207

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

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

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

236288
#
237289
# Creates a wrapper for a function which records the time it takes to finish
@@ -6612,6 +6664,23 @@ def test_preload_main_sys_argv(self):
66126664
'',
66136665
])
66146666

6667+
@only_run_in_forkserver_testsuite("forkserver specific test.")
6668+
def test_preload_main_large_sys_argv(self):
6669+
# gh-144503: a very large parent sys.argv must not prevent the
6670+
# forkserver from starting (it previously overflowed the OS
6671+
# per-argument length limit when repr'd into the -c command string).
6672+
name = os.path.join(os.path.dirname(__file__),
6673+
'mp_preload_large_sysargv.py')
6674+
_, out, err = test.support.script_helper.assert_python_ok(name)
6675+
self.assertEqual(err, b'')
6676+
6677+
out = out.decode().split("\n")
6678+
self.assertEqual(out, [
6679+
'preload:5002:sentinel',
6680+
'worker:5002:sentinel',
6681+
'',
6682+
])
6683+
66156684
#
66166685
# Mixins
66176686
#
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)