potential undefined behavior with subprocess using vfork() on Linux?

[socketpair]

BPO	47245
Nosy	@gpshead, @socketpair, @izbyshev

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2022-04-08.13:33:50.215>
created_at = <Date 2022-04-06.20:24:52.654>
labels = ['extension-modules', 'type-bug', '3.10', '3.11']
title = 'potential undefined behavior with subprocess using vfork() on Linux?'
updated_at = <Date 2022-04-08.13:33:50.214>
user = 'https://github.com/socketpair'

bugs.python.org fields:

activity = <Date 2022-04-08.13:33:50.214>
actor = 'socketpair'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2022-04-08.13:33:50.215>
closer = 'socketpair'
components = ['Extension Modules']
creation = <Date 2022-04-06.20:24:52.654>
creator = 'socketpair'
dependencies = []
files = []
hgrepos = []
issue_num = 47245
keywords = ['3.10regression']
message_count = 13.0
messages = ['416897', '416898', '416899', '416900', '416904', '416905', '416909', '416917', '416946', '416964', '416974', '416976', '416984']
nosy_count = 3.0
nosy_names = ['gregory.p.smith', 'socketpair', 'izbyshev']
pr_nums = []
priority = 'normal'
resolution = 'works for me'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue47245'
versions = ['Python 3.10', 'Python 3.11']

[socketpair]

Using vfork in bpo-35823 is VERY tricky. Please comment out vfork() usage for now. Yes, we can (should) use vfork(), but we have to rewrite the child code.

https://bugzilla.kernel.org/show_bug.cgi?id=215813

I would say it's URGENT.

[socketpair]

Rewriting in a way that guarantee no stack (and heap) usage. Because stack is shared between child and parent. It seems there is no crossplatform way. Happily we can use some code like I wrote by link in the first message. OR, stick to posix_nspawn which is cross-platform.

[socketpair]

cpython/Modules/_posixsubprocess.c

Line 717 in 4a08c4c

child_exec(exec_array, argv, envp, cwd,

child_exec(exec_array, argv, envp, cwd,
           p2cread, p2cwrite, c2pread, c2pwrite,
           errread, errwrite, errpipe_read, errpipe_write,
           close_fds, restore_signals, call_setsid,
           call_setgid, gid, call_setgroups, groups_size, groups,
           call_setuid, uid, child_umask, child_sigmask,
           py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);

----------------

Py_NO_INLINE static void
child_exec(char *const exec_array[],
char *const argv[],
char *const envp[],
const char *cwd,
int p2cread, int p2cwrite,
int c2pread, int c2pwrite,

-------------

So, calling child_exec pushes to stack GUARANTEED. This is the bug. In fact everything works, but is too weak and break at any moment.

Again. Please comment calling vfork() for now. And re-implement child part later.

[socketpair]

Solution:

https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/spawni.c#L309

In short - do not use vfork(). Use clone(CLONE_VM | CLONE_VFORK). and do something with stack.

[gpshead]

Can you provide a reproducable way to demonstrate evidence of a problem in CPython's use of the Linux libc vfork() implementation? A test case that causes a CPython parent or child process on Linux when built with HAVE_VFORK failing to function properly would help prioritize this because in practice nobody has reported problems in 3.10.

(we've deployed the subprocess vfork code into thousands production Python programs at work in the past year w/o issues being reported - though we have a constrained environment with use on only a couple of libc versions and limited set of kernels on a few very common architectures)

General thinking (possible dated and incorrect - against what https://man7.org/linux/man-pages/man2/vfork.2.html wording claims with its "or calls any other function" text):

Pushing additional data onto the stack in the child process _should_ not a problem. That by definition lands in previously unused pre-allocated stack space. If that page faults, that could map a new page into the process state shared by both the paused parent and running child. But this page mapping should be fine - the child exec that resumes the parent means the parent is the only one who sees it.

When the parent process resumes, sure, that data will be in that memory on the unallocated portion of stack, but critically the *stack pointer* in the parent process (a register) never changes. As far as I understand things, registers are not shared between vfork()ed processes. So the parent only sees some temporary data having been written to the unused region of the stack by the since-replaced by exec() child process. No big deal.

**Untrue wishful thinking**: If a new stack were needed on a given platform for use in the vfork()ed child, I'd like it to be the job of libc to take care of that for us. glibc sources do no such thing (every vfork supporting architecture has a vfork.S code that appears to make the syscall and jump back to the caller without stack manipulation).

[gpshead]

Immediate action item: Add a way for people to disable vfork at runtime by setting a flag in the subprocess module, just in case.

This can be backported to 3.10 - It'd provide an escape hatch for anyone without a need to rebuild Python to disable use of vfork() without resorting to LD_PRELOAD hacks.

This is not an urgent issue unless actual practical problems are being observed in the field.

[gpshead]

Our current assumptions around the use of vfork() are very much glibc specific.

Another useful reference for reasoning, comments, and history is https://github.com/golang/go/blob/master/src/syscall/exec_linux.go#L146 forkAndExecInChild1

[izbyshev]

In short: both this bug report and [1] are invalid.

The reason why doing syscall(SYS_vfork) is illegal is explained by Florian Weimer in [2]:

The syscall function in glibc does not protect the on-stack return address against overwriting, so it can't be used to call SYS_vfork on x86.

This is off-topic here because CPython calls vfork() via libc, but I'll still expand the Florian's comment. Suppose one wants to write my_vfork() wrapper over vfork syscall. When vfork syscall returns the first time, my_vfork() has to return to its caller. This necessarily involves knowing the return address. On some architectures this return address is stored on the stack by the caller (e.g. x86). The problem is that any data in my_vfork() stack frame can be overwritten by its caller once it returns the first time. Then, when vfork syscall returns the second time, my_vfork() could be unable to return to its caller because the data it fetches from its (now invalid) stack frame is garbage. This is precisely what happens when one implements my_vfork() as syscall(SYS_vfork). To avoid this, the most common strategy is to store the return address into a register that's guaranteed to be preserved around syscall by the OS ABI. For example, the x86-64 musl implementation [3] stores the return address in rdx (which is preserved around syscall) and then restores it after syscall (both on the first and the second return of the syscall).

Now back to CPython. The real problem with stack sharing between the child and the parent could be due to compiler bugs, e.g. if a variable stored on the stack is modified in the child branch of "if (vfork())", but the compiler assumes that some other variable sharing the stack slot with the first one is *not* modified in the parent branch (i.e. after vfork() returns the second time). But all production compilers handle vfork() (and setjmp(), which has a similar issue) in a special way to avoid this, and GCC has __attribute__((returns_twice)) that a programmer could use for custom functions behaving in this way (my_vfork() would have to use this attribute).

Regarding a separate stack for the child and clone(CLONE_VM|CLONE_VFORK), I considered this in bpo-35823, but this has its own problems. The most important one is that CPython would be in business of choosing the correct stack size for the child's stack, but CPython is *not* in control of all the code executing in the child because it calls into libc. In practice, people use various LD_PRELOAD-based software that wraps various libc functions (e.g. Scratchbox 2 build environment for Sailfish OS is an LD_PRELOAD-based sandbox), so even common-sense assumptions like "execve() in libc can't use a lot of stack!" might turn out to be wrong. CPython *could* work around that by using direct syscalls for everything in the child, or by using some "large" size that "should be enough for everybody", but I wouldn't want to see that unless we have a real problem with vfork(). Note that vfork()-based posix_spawn() implementations in C libraries do *not* have this problem because they fully control all code in the child (e.g. they would use a non-interposable execve() symbol or a direct syscall).

Immediate action item: Add a way for people to disable vfork at runtime by setting a flag in the subprocess module, just in case.

I don't think any action is needed at all, and I think this issue should be closed.

Our current assumptions around the use of vfork() are very much glibc specific.

Could you clarify what glibc-specific assumptions you mean? In bpo-35823 I tried to use as little assumptions as possible.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=215813
[2] https://bugzilla.kernel.org/show_bug.cgi?id=215813#c2
[3] https://git.musl-libc.org/cgit/musl/tree/src/process/x86_64/vfork.s?id=ced75472d7e3d73d5b057e36ccbc7b7fcba95104

[gpshead]

Thanks! I agree with you that this is probably not an actual problem on Linux.

_I did look at the various glibc architecture vfork.s implementations: Cute tricks used on some where they need to avoid a stack modifying traditional return from vfork()._

As for glibc specifics, I'm mostly thinking of the calls we do in the child.

According to the "Standard Description (POSIX.1)" calls to anything other than _exit() or exec*() are not allowed. But the longer "Linux Description" in that vfork(2) man page does not say that. Which implies merely by omission that calls to other things are okay so long as you understand everything they do to the process heap/stack/state. (I wish it were explicit about that)

Some of the calls we do from our child_exec() code... many are likely "just" syscall shims and thus fine - but that is technically up to libc.

A few others are Py functions that go elsewhere in CPython and while they may be fine for practical reasons today with dangerous bits on conditional branches that technically should not be possible to hit given the state by the time we're at this point in _posixsubprocess, pose a future risk - anyone touching implementations of those is likely unaware of vfork'ed child limitations that must be met.

For example if one of the potential code paths that trigger an indirect Py_FatalError() is hit... that fatal exit code is definitely not post-vfork-child safe. The pre-exec child dying via that could screw up the vfork parent process's state.

[izbyshev]

As for glibc specifics, I'm mostly thinking of the calls we do in the child.

According to the "Standard Description (POSIX.1)" calls to anything other than _exit() or exec*() are not allowed. But the longer "Linux Description" in that vfork(2) man page does not say that. Which implies merely by omission that calls to other things are okay so long as you understand everything they do to the process heap/stack/state. (I wish it were explicit about that)

If we're talking about the kernel side of things, sure, we rely on Linux being "sane" here, though I suppose on *BSDs the situation is similar.

Some of the calls we do from our child_exec() code... many are likely "just" syscall shims and thus fine - but that is technically up to libc.

Yes, but I wouldn't say that "being just syscall shims" is specific for glibc. It's just a "natural" property that just about any libc is likely to possess. (Yeah, I know, those are vague words, but in my experience "glibc-specific" is usually applied to some functionality/bug present in glibc and absent in other libcs, and I don't think we rely on something like that).

Of course, there are also LD_PRELOAD things that could be called instead of libc, but good news here is that we don't create new constrains for them (CPython is not the only software that uses vfork()), and they're on their own otherwise.

A few others are Py functions that go elsewhere in CPython and while they may be fine for practical reasons today with dangerous bits on conditional branches that technically should not be possible to hit given the state by the time we're at this point in _posixsubprocess, pose a future risk - anyone touching implementations of those is likely unaware of vfork'ed child limitations that must be met.

We already have async-signal-safety requirement for all such code because of fork(). Requirements of vfork() are a bit more strict, but at least the set of functions we have to watch for dangerous changes is the same. And I suspect that most practical violations of vfork()-safety also violate async-signal-safety.

For example if one of the potential code paths that trigger an indirect Py_FatalError() is hit... that fatal exit code is definitely not post-vfork-child safe. The pre-exec child dying via that could screw up the vfork parent process's state.

Yeah, and it can break the fork parent too, at least because it uses exit() (not _exit()), so stdio buffers will be flushed twice, in the child and in the parent.

[socketpair]

So, finally:

1. Regarding vfork() and stack - everything is nice. No bugs because libc has nasty hacks for stack restoration.

2. Having the ability to turn off vfork using environment variables is NICE. At least, one can easily compare the performance.

3. We have to fix error-path in order not to change heap state (contents and allocations), possibly do not touch locks. During vfork() child execution - the only parent THREAD (not the process) is blocked. For example, it's not allowed to touch GIL. Child process may die unexpectedly and leave GIL locked. Is it possible to rewrite children path for vfork() case without any Py* calls ? As an idea, we can prepare all low-level things (all the pointers to strings and plain values) before vfork(), so child code will use only that data.

[izbyshev]

3. We have to fix error-path in order not to change heap state (contents and allocations), possibly do not touch locks. During vfork() child execution - the only parent THREAD (not the process) is blocked. For example, it's not allowed to touch GIL. Child process may die unexpectedly and leave GIL locked. Is it possible to rewrite children path for vfork() case without any Py* calls ? As an idea, we can prepare all low-level things (all the pointers to strings and plain values) before vfork(), so child code will use only that data.

What specifically do you propose to fix? There is no problem with GIL if the child dies because the GIL is locked and unlocked only by the parent and the child never touches it. Similarly, only Py_* calls known to be safe are used. As for "pointers to strings", it's not clear to me what you mean, but if you mean allocations, they are already done before (v)fork(), since the child code is required to be async-signal-safe even if plain fork() is used.

[socketpair]

I have studied assembler output of _posixsubprocess.o compilation. Yes, everything seems safe. So, I'm closing the bug.

[gpshead]

I'm reopening this to track adding a failsafe so people have a big-red-button style way to disable Python's use of vfork without recompiling just in case of future problems. That arguably should've been there from the start in 3.10. Though we still have no known need to use it, by the time you do it needs to already exist.

I'll do a manual more conservative backport of the disable_vfork_reason button to 3.10 for 3.10.5.

[vstinner]

I wish posix_spawn() could be used in more cases on Linux: see #86904.

[izbyshev]

I wish posix_spawn() could be used in more cases on Linux: see #86904.

On the contrary, I wish posix_spawn() completely disabled on Linux. Using different backends depending on Popen() arguments only adds possibility for hard-to-understand behavioral differences. And posix_spawn() in glibc is still buggy (#91307 (comment)).

[MojoVampire]

@gpshead: It looks like you added the big-red-button; can this be closed?

[gpshead]

On the contrary, I wish posix_spawn() completely disabled on Linux. Using different backends depending on Popen() arguments only adds possibility for hard-to-understand behavioral differences. And posix_spawn() in glibc is still buggy (#91307 (comment)).

Hah, joy, but not surprised, that at least proves that failsafes knobs to let modern things be disabled have uses.

The 3.10 PR being merged should close this issue out.

[arthur-proglove]

Hi,

Sorry to dig up this old issue. We are trying to figure out a kernel crash in our Python application and figure out it's always coming from sys_vfork.

Python: 3.11.2
System: Debian 12 on a Raspberry Pi Zero 2W
Kernel: v6.6.47

I have not tried to unset _USE_VFORK but maybe it could be related?

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000014 when read
[00000014] *pgd=03f2c835, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in: xt_recent usb_f_ecm u_ether usb_f_mass_storage usb_f_acm u_serial dwc2 roles cmac algif_hash aes_arm_bs crypto_simd cryptd algif_skcipher af_alg btnxpuart bluetooth ecdh_generic ecc crc8 moal(O) mlan(O) xt_hl cfg80211 ip6t_rt rfkill ipt_REJECT nf_reject_ipv4 xt_LOG nf_log_syslog nft_limit xt_limit xt_addrtype xt_tcpudp xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nft_compat nf_tables nfnetlink binfmt_misc sc16is7xx regmap_i2c raspberrypi_hwmon i2c_bcm2835 raspberrypi_gpiomem fixed uio_pdrv_genirq uio panel_waveshare_dsi w5100_spi w5100 libcomposite i2c_dev deflate zstd ubifs ubi ofpart spi_nor mtd spi_bcm2835 drm fuse drm_panel_orientation_quirks backlight ip_tables x_tables ipv6 overlay
CPU: 1 PID: 484 Comm: python3 Tainted: G           O       6.6.47-v7 #1
Hardware name: BCM2835
PC is at memcg_charge_kernel_stack+0xc/0x9c
LR is at copy_process+0xcc8/0x1d5c
pc : [<80119d90>]    lr : [<8011c448>]    psr: 60000013
sp : 9ff51e10  ip : 00000000  fp : 00000000
r10: 9a7d5d7c  r9 : ffffffff  r8 : 9ff51ecc
r7 : 84b1d8c0  r6 : 9ff51f48  r5 : 7ea183c8  r4 : 8100e90c
r3 : 00002a11  r2 : 00002a10  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c5383d  Table: 05c5806a  DAC: 00000055
Register r0 information: NULL pointer
Register r1 information: NULL pointer
Register r2 information: non-paged memory
Register r3 information: non-paged memory
Register r4 information: non-slab/vmalloc memory
Register r5 information: non-paged memory
Register r6 information: 2-page vmalloc region starting at 0x9ff50000 allocated at kernel_clone+0xac/0x3a8
Register r7 information: slab task_struct start 84b1d8c0 pointer offset 0 size 4544
Register r8 information: 2-page vmalloc region starting at 0x9ff50000 allocated at kernel_clone+0xac/0x3a8
Register r9 information: non-paged memory
Register r10 information: non-slab/vmalloc memory
Register r11 information: NULL pointer
Register r12 information: NULL pointer
Process python3 (pid: 484, stack limit = 0x0af81d55)
Stack: (0x9ff51e10 to 0x9ff52000)
1e00:                                     8100e90c 7ea183c8 9ff51f48 8011c448
1e20: 00000dc2 0000065f 00000000 ffffffff 8011d624 80ace5f8 9ff51e98 00004100
1e40: 821d8000 9ff51f48 00000000 00000000 00000000 00000000 8116b41c 00000000
1e60: 00000000 00000000 9a7d5d7c 005659e8 00000000 801b9424 84b1d8c0 801d1d98
1e80: 00000000 00000000 9ef8e548 00000000 012f8586 8172d880 9ff51e98 00000000
1ea0: 9ff51f00 9ff51fb0 00000000 00000000 9ff51ee0 00000008 00000000 84b1d8c0
1ec0: 00000008 802e1b80 84b1d8c0 00000000 00000000 00000000 84a89538 218c150c
1ee0: 00000000 00004100 7ea183c8 9ff51f48 000000be 00000000 84b1d8c0 000000be
1f00: 00000000 8011d624 00000001 00000000 00000000 00000000 00000000 218c150c
1f20: 00000089 00000000 7ea183c8 70ab1b40 000000be 80100298 84b1d8c0 000000be
1f40: 00000000 8011dc2c 00004100 00000000 00000000 00000000 00000000 00000000
1f60: 00000011 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1f80: 00000000 00000000 00000000 00000000 00000000 00000000 7ea183c8 218c150c
1fa0: 7fffffff 80100040 00000000 7ea183c8 72d982d8 70ab1b40 00000000 00000000
1fc0: 00000000 7ea183c8 70ab1b40 000000be 00000000 00000000 00000000 00000000
1fe0: 72d982d8 7ea1826c 00273d80 76d91320 20000010 72d982d8 00000000 00000000
 memcg_charge_kernel_stack from copy_process+0xcc8/0x1d5c
 copy_process from kernel_clone+0xac/0x3a8
 kernel_clone from sys_vfork+0x4c/0x70
 sys_vfork from ret_fast_syscall+0x0/0x4c
Exception stack(0x9ff51fa8 to 0x9ff51ff0)
1fa0:                   00000000 7ea183c8 72d982d8 70ab1b40 00000000 00000000
1fc0: 00000000 7ea183c8 70ab1b40 000000be 00000000 00000000 00000000 00000000
1fe0: 72d982d8 7ea1826c 00273d80 76d91320
Code: ea0168e2 e92d4070 e52de004 e28dd004 (e5903014) 
---[ end trace 0000000000000000 ]---

[vstinner]

Sorry to dig up this old issue.

This issue is closed. I suggest you to open a new issue.