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

[Sylvain Pineau]

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()?

[Maciej Kisielewski (kissiel)]

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 on 2019-09-08

execution: Use proc.communicate() to supply password

The additional pipe creates a deadlock with commands like:

$ more foo | cat

Fixes: lp:1841702

diff --git a/plainbox/impl/execution.py b/plainbox/impl/execution.py
index 01e783f..740d4e7 100644
--- a/plainbox/impl/execution.py
+++ b/plainbox/impl/execution.py
@@ -182,45 +182,12 @@ class UnifiedRunner(IJobRunner):
 
         def call(extcmd_popen, *args, **kwargs):
             """Handle low-level subprocess stuff."""
-            is_alive = True
             # Notify that the process is about to start
             extcmd_popen._delegate.on_begin(args, kwargs)
             # Setup stdout/stderr redirection
             kwargs['stdout'] = subprocess.PIPE
             kwargs['stderr'] = subprocess.PIPE
             kwargs['start_new_session'] = True
-            # Prepare stdio supply
-            in_r, in_w = os.pipe()
-            # first let's punch the password in
-            # we need it only if the target user differs from the one that
-            # started checkbox and when changing the user (sudo) requires
-            # password
-            if target_user and self._password_provider:
-                password = self._password_provider()
-                if password:
-                    os.write(in_w, password + b'\n')
-
-            def stdin_forwarder(stdin):
-                """Forward data from one pipe to the other."""
-                # use systems stdin if the stdin pipe wasn't provided
-                stdin = stdin or sys.stdin
-                try:
-                    while is_alive:
-                        if stdin in select.select([stdin], [], [], 0)[0]:
-                            buf = stdin.readline()
-                            if buf == '':
-                                os.close(in_w)
-                                break
-                            os.write(in_w, buf.encode(stdin.encoding))
-                        else:
-                            time.sleep(0.1)
-                except BrokenPipeError:
-                    pass
-            forwarder_thread = threading.Thread(
-                target=stdin_forwarder, args=(stdin,))
-            forwarder_thread.start()
-            kwargs['stdin'] = in_r
-
             # Start the process
             proc = extcmd_popen._popen(*args, **kwargs)
             self._running_jobs_pid = proc.pid
@@ -238,7 +205,10 @@ class UnifiedRunner(IJobRunner):
             try:
                 while True:
                     try:
-                        proc.wait()
+                        pass_bytes=None
+                        if target_user and self._password_provider:
+                            pass_bytes = self._password_provider() + b'\n'
+                        proc.communicate(input=pass_bytes)
                         break
                     except KeyboardInterrupt:
                         # On interrupt send a signal to the process
@@ -255,9 +225,6 @@ class UnifiedRunner(IJobRunner):
                 # Tell the queue worker to shut down
                 extcmd_popen._queue.put(None)
                 queue_worker.join()
-                os.close(in_r)
-                is_alive = False
-                forwarder_thread.join()
             # Notify that the process has finished
             extcmd_popen._delegate.on_end(proc.returncode)
             return proc.returncode