saltutil.runner/wheel privilege-drop child can hang, breaks runners that spawn processes, and flattens exception types
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
saltuser (3006 default), minion runs
asroot, 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'exceptclauses 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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