Merge ~sylvain-pineau/checkbox-ng:fix-1841702 into checkbox-ng:master

Proposed by Sylvain Pineau
Status: Rejected
Rejected by: Sylvain Pineau
Proposed branch: ~sylvain-pineau/checkbox-ng:fix-1841702
Merge into: checkbox-ng:master
Diff against target: 72 lines (+4/-37)
1 file modified
plainbox/impl/execution.py (+4/-37)
Reviewer Review Type Date Requested Status
Maciej Kisielewski (community) Disapprove
Checkbox Developers Pending
Review via email: mp+372452@code.launchpad.net

Description of the change

Fixes the linked bug.

The new unified runner introduced and intermediate player, stdin_forwarder() which creates a deadlock with commands like:

$ more foo | cat

The proposed patch is using a method originally found in RootViaSudoWithPassExecutionController->execute_job() [1]

[1] can be found in the last stable release: https://launchpad.net/checkbox-ng/trunk/1.4.0/+download/checkbox-ng-1.4.0.tar.gz

Maciej, could you please check I'm not breaking remote features by annihilating this stdin_forwarder()?

To post a comment you must log in.
Revision history for this message
Maciej Kisielewski (kissiel) wrote :

stdin is not only for supplying password, but also for sending input to jobs that read stdin.
Like collect manifest for instance. So we need the red bit, I'm affraid.

review: Disapprove

Unmerged commits

f03ad1e... by Sylvain Pineau

execution: Use proc.communicate() to supply password

The additional pipe creates a deadlock with commands like:

$ more foo | cat

Fixes: lp:1841702

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/plainbox/impl/execution.py b/plainbox/impl/execution.py
2index 01e783f..740d4e7 100644
3--- a/plainbox/impl/execution.py
4+++ b/plainbox/impl/execution.py
5@@ -182,45 +182,12 @@ class UnifiedRunner(IJobRunner):
6
7 def call(extcmd_popen, *args, **kwargs):
8 """Handle low-level subprocess stuff."""
9- is_alive = True
10 # Notify that the process is about to start
11 extcmd_popen._delegate.on_begin(args, kwargs)
12 # Setup stdout/stderr redirection
13 kwargs['stdout'] = subprocess.PIPE
14 kwargs['stderr'] = subprocess.PIPE
15 kwargs['start_new_session'] = True
16- # Prepare stdio supply
17- in_r, in_w = os.pipe()
18- # first let's punch the password in
19- # we need it only if the target user differs from the one that
20- # started checkbox and when changing the user (sudo) requires
21- # password
22- if target_user and self._password_provider:
23- password = self._password_provider()
24- if password:
25- os.write(in_w, password + b'\n')
26-
27- def stdin_forwarder(stdin):
28- """Forward data from one pipe to the other."""
29- # use systems stdin if the stdin pipe wasn't provided
30- stdin = stdin or sys.stdin
31- try:
32- while is_alive:
33- if stdin in select.select([stdin], [], [], 0)[0]:
34- buf = stdin.readline()
35- if buf == '':
36- os.close(in_w)
37- break
38- os.write(in_w, buf.encode(stdin.encoding))
39- else:
40- time.sleep(0.1)
41- except BrokenPipeError:
42- pass
43- forwarder_thread = threading.Thread(
44- target=stdin_forwarder, args=(stdin,))
45- forwarder_thread.start()
46- kwargs['stdin'] = in_r
47-
48 # Start the process
49 proc = extcmd_popen._popen(*args, **kwargs)
50 self._running_jobs_pid = proc.pid
51@@ -238,7 +205,10 @@ class UnifiedRunner(IJobRunner):
52 try:
53 while True:
54 try:
55- proc.wait()
56+ pass_bytes=None
57+ if target_user and self._password_provider:
58+ pass_bytes = self._password_provider() + b'\n'
59+ proc.communicate(input=pass_bytes)
60 break
61 except KeyboardInterrupt:
62 # On interrupt send a signal to the process
63@@ -255,9 +225,6 @@ class UnifiedRunner(IJobRunner):
64 # Tell the queue worker to shut down
65 extcmd_popen._queue.put(None)
66 queue_worker.join()
67- os.close(in_r)
68- is_alive = False
69- forwarder_thread.join()
70 # Notify that the process has finished
71 extcmd_popen._delegate.on_end(proc.returncode)
72 return proc.returncode

Subscribers

People subscribed via source and target branches