Comment 1 for bug 1961459

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Yeah, I don’t really think those are security issues, let’s open it, but fix some of them:

-> the 2 arggv leak in fork()
This is only if a failure and just before exiting the child process. Anyway, let’s free it :)

-> status != 0
Yes, we could have done something like the man page:
               do {
                   w = waitpid(cpid, &wstatus, WUNTRACED | WCONTINUED);
                   if (w == -1) {
                       perror("waitpid");
                       exit(EXIT_FAILURE);
                   }

                   if (WIFEXITED(wstatus)) {
                       printf("exited, status=%d\n", WEXITSTATUS(wstatus));
                   } else if (WIFSIGNALED(wstatus)) {
                       printf("killed by signal %d\n", WTERMSIG(wstatus));
                   } else if (WIFSTOPPED(wstatus)) {
                       printf("stopped by signal %d\n", WSTOPSIG(wstatus));
                   } else if (WIFCONTINUED(wstatus)) {
                       printf("continued\n");
                   }
               } while (!WIFEXITED(wstatus) && !WIFSIGNALED(wstatus));

However, this seems equivalent to me. We took our inspiration from pam_exec() with the different kinds of errors and this is the way they are using it, so we did something similar as we are running in the same context. It ought to work as we tested different kind of failures manually and used scenarios worked. Note that pam_exec don’t even free argvv for the parent process :p
So, depending on how strongly you feel about it, tell us!

-> gethostname() indentation
typical case of "this was looking fine in my editor" :p This is why I like Go with autoformatting :)

arggv leak and gethostname identation fixed in https://github.com/ubuntu/adsys/pull/285.