Fixed checking exit status when calling 'sudo'

There were a few closely related issues in how the code tried to decide
whether running 'sudo' succeeded:

The first issue I noticed was that in some cases, the wait() call would
fail with ECHILD. What happened was that the waitpid() call noticed that
the sudo process terminated and reaped the zombie. Then, the loop was
left and wait() was called on the PID again -- at this point, there was
no child with that PID anymore though.

The user-visible consequence of this was that the authentication was
incorrectly considered to have failed, and a "Cannot get authorization"
dialog was shown even though the correct root password was entered.

No matter whether wait() failed or not, the code would try to use the
WIFEXITED() and WEXITSTATUS() macros to interpret the last status. This
caused some reading of uninitialized memory (as Valgrind pointed out),
since WIFEXITED() may only be called in case wait() succeeded. Furthermore,
as wait(2) explains:

  WEXITSTATUS(status)
    returns the exit status of the child.  [..] This macro should be employed
    only if WIFEXITED returned true.

This patch fixes all these problems by

  a) not calling wait() again but rather using the status which was
     fetched by waitpid() and
  b) only using the W* macros if it's legal to do so (i.e. waitpid()
     succeeded).

Change-Id: I81741898dc686bb2d2128fceb9e71535fab43417
Reviewed-by: Karsten Heimrich <karsten.heimrich@theqtcompany.com>

1 files changed, 10 insertions, 10 deletions

diff --git a/src/libs/installer/adminauthorization_x11.cpp b/src/libs/installer/adminauthorization_x11.cpp
index f451f344e..f3372ddd2 100644
--- a/src/libs/installer/adminauthorization_x11.cpp
+++ b/src/libs/installer/adminauthorization_x11.cpp
@@ -162,10 +162,17 @@ bool AdminAuthorization::execute(QWidget *parent, const QString &program, const
         int errBytes = 0;
         char buf[1024];
         char errBuf[1024];
+        int status;
+        bool statusValid = false;
         while (bytes >= 0) {
-            int state;
-            if (::waitpid(child, &state, WNOHANG) == -1)
+            const pid_t waitResult = ::waitpid(child, &status, WNOHANG);
+            if (waitResult == -1) {
+                break;
+            }
+            if (waitResult == child) {
+                statusValid = true;
                 break;
+            }
             bytes = ::read(masterFD, buf, 1023);
             if (bytes == -1 && errno == EAGAIN)
                 bytes = 0;
@@ -203,15 +210,8 @@ bool AdminAuthorization::execute(QWidget *parent, const QString &program, const
             return false;
         }
 
-        int status;
-        child = ::wait(&status);
-        const int exited = WIFEXITED(status);
-        const int exitStatus = WEXITSTATUS(status);
         ::close(pipedData[1]);
-        if (exited)
-            return exitStatus == 0;
-
-        return false;
+        return statusValid && WIFEXITED(status) && WEXITSTATUS(status) == 0;
     }
 
     // child process