saltstack / saltstack/salt

saltutil.runner/wheel privilege-drop child can hang, breaks runners that spawn processes, and flattens exception types

Open
#69,618 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
15.7k
Forks
5.6k
Avg merge
2d 44m
Merged PRs (30d)
80

Description

Description

#69240 (fix for #67716) made saltutil.runner/saltutil.wheel drop to the
master's configured user by running the runner/wheel in a short-lived forked
child (_client_cmd_as), returning the result over a multiprocessing.Queue.
The privilege drop itself is correct, but the child/queue plumbing has three
robustness problems. They only trigger on the drop path -- current process is
root and the master runs as a different real user, i.e. the packaged 3006
default (salt master + colocated root minion), and via the scheduler on
such a minion.

1. The child is daemon=True, so runners/wheels that spawn their own
processes fail.
A daemonic process is not allowed to have children, so any
runner that starts a multiprocessing.Process raises
AssertionError: daemonic processes are not allowed to have children
(surfaced as CommandExecutionError). The concrete in-tree case is
saltutil.runner state.orchestrate for an SLS containing a parallel: True
state (salt/state.py call_parallel -> salt.utils.process.Process). These
calls ran in-process before #69240.

2. queue.get() has no timeout and the child's liveness is never checked, so
a child that dies before returning a result hangs the caller forever.
If the
child is OOM-killed, os._exits, or segfaults in a C extension (libgit2/pygit2
during git_pillar.update -- the motivating workload of #67716), it never puts
a result and the parent blocks on queue.get() indefinitely. proc.join() in
the finally is unreachable. There is no wall-clock reaper, so the minion-side
job is stuck permanently. (A return value that cannot be pickled onto the queue
has the same effect: the feeder thread dies and the parent blocks.)

3. Every child exception is flattened to CommandExecutionError, losing the
original type and traceback.
The drop path therefore behaves differently from
the in-process path. In particular wheel()'s except SaltInvocationError
handler can never fire on the drop path, because the child's SaltInvocationError
is re-raised as CommandExecutionError before wheel() sees it.

Setup
  • onedir packaging; master runs as the salt user (3006 default), minion runs
    as root, colocated on the same host. (Same topology as #67716 / #69569 /
    #69600.)
Steps to reproduce the behavior

Minimal, master/minion-free reproduction of the child plumbing, run as root
(drops to nobody for real):

# repro.py -- run: sudo <onedir-python> repro.py
import multiprocessing, os, signal, threading
import salt.modules.saltutil as saltutil
from salt.exceptions import CommandExecutionError, SaltInvocationError

def guarded(fn, secs=20):
    box = {}
    def run():
        try: box["r"] = ("ok", fn())
        except BaseException as e: box["r"] = ("exc", e)
    t = threading.Thread(target=run, daemon=True); t.start(); t.join(secs)
    return ("HANG", None) if t.is_alive() else box["r"]

class Spawn:                     # problem 1
    functions = {}
    def cmd(self, name, **kw):
        ctx = multiprocessing.get_context("fork"); q = ctx.Queue()
        p = ctx.Process(target=lambda q: q.put("ok"), args=(q,))
        p.start(); r = q.get(); p.join(); return r
class Crash:                     # problem 2
    functions = {}
    def cmd(self, name, **kw): os.kill(os.getpid(), signal.SIGKILL)
class Raise:                     # problem 3
    functions = {}
    def cmd(self, name, **kw): raise SaltInvocationError("bad args")

print("spawn:", guarded(lambda: saltutil._client_cmd_as("nobody", Spawn(), "x", {})))
print("crash:", guarded(lambda: saltutil._client_cmd_as("nobody", Crash(), "x", {})))
print("raise:", guarded(lambda: saltutil._client_cmd_as("nobody", Raise(), "x", {})))

Stock 3006.26 prints (problem 1 = daemonic error, problem 2 = HANG,
problem 3 = wrong type):

spawn: ('exc', CommandExecutionError("Failed to run 'x' as user 'nobody': AssertionError: daemonic processes are not allowed to have children"))
crash: ('HANG', None)
raise: ('exc', CommandExecutionError("Failed to run 'x' as user 'nobody': SaltInvocationError: bad args"))

Realistic reproduction through saltutil.runner on a salt-master + root-minion,
with two tiny runners in runner_dirs (spawnr.spawn starts a process,
crashr.boom does os.kill(os.getpid(), signal.SIGKILL)):

# salt <minion> saltutil.runner spawnr.spawn
    ... AssertionError: daemonic processes are not allowed to have children
# salt <minion> saltutil.runner state.orchestrate arg="[orch.par]"   # SLS has a parallel:True state
    ... AssertionError: daemonic processes are not allowed to have children
# salt <minion> saltutil.runner crashr.boom
    <hangs until the CLI --timeout; the minion-side job never returns>
Expected behavior
  • A runner/wheel that spawns its own processes runs normally under the drop.
  • A child that dies without returning a result raises CommandExecutionError
    promptly instead of hanging.
  • The drop path raises the same exception type the in-process path would, so
    callers' except clauses keep working.
Versions Report

Present on 3006.x since #69240; merges forward to 3007.x / master (which carry
_client_cmd_as unchanged). Reproduced on 3006.26 (Debian 12, Python 3.11).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with salt.modules.saltutil._client_cmd_as and the process path described in the issue; inspect salt/state.py call_parallel for the nested-process case. Run repro.py under the stated privilege-drop setup and verify that child-spawning runners work, crashed children fail promptly, and SaltInvocationError preserves its type and traceback.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, infrastructure
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.