reduce scope of variable. Found by PVS-Studio
[freeradius.git] / src / main / exec.c
index f200255..d5b3a9a 100644 (file)
@@ -1,8 +1,4 @@
 /*
- * exec.c      Execute external programs.
- *
- * Version:    $Id$
- *
  *   This program is free software; you can redistribute it and/or modify
  *   it under the terms of the GNU General Public License as published by
  *   the Free Software Foundation; either version 2 of the License, or
  *   You should have received a copy of the GNU General Public License
  *   along with this program; if not, write to the Free Software
  *   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+
+/*
+ * $Id$
  *
- * Copyright 2000-2004,2006  The FreeRADIUS server project
+ * @file exec.c
+ * @brief Execute external programs.
+ *
+ * @copyright 2000-2004,2006  The FreeRADIUS server project
  */
 
-#include <freeradius-devel/ident.h>
 RCSID("$Id$")
 
-#include <freeradius-devel/autoconf.h>
+#include <freeradius-devel/radiusd.h>
+#include <freeradius-devel/rad_assert.h>
 
 #include <sys/file.h>
 
-#include <stdlib.h>
-#include <string.h>
 #include <fcntl.h>
 #include <ctype.h>
-#include <signal.h>
-
-#ifdef HAVE_UNISTD_H
-#include <unistd.h>
-#endif
 
 #ifdef HAVE_SYS_WAIT_H
 #      include <sys/wait.h>
@@ -47,183 +43,157 @@ RCSID("$Id$")
 #      define WIFEXITED(stat_val) (((stat_val) & 255) == 0)
 #endif
 
-#include <freeradius-devel/radiusd.h>
-#include <freeradius-devel/rad_assert.h>
-
 #define MAX_ARGV (256)
-/*
- *     Execute a program on successful authentication.
- *     Return 0 if exec_wait == 0.
- *     Return the exit code of the called program if exec_wait != 0.
- *     Return -1 on fork/other errors in the parent process.
- */
-int radius_exec_program(const char *cmd, REQUEST *request,
-                       int exec_wait,
-                       char *user_msg, int msg_len,
-                       VALUE_PAIR *input_pairs,
-                       VALUE_PAIR **output_pairs,
-                       int shell_escape)
-{
-       VALUE_PAIR *vp;
-       char mycmd[1024];
-       char answer[4096];
-       char argv_buf[4096];
-       char *argv[MAX_ARGV];
-       const char *from;
-       char *p, *to;
-       int pd[2];
-       pid_t pid, child_pid;
-       int argc = -1;
-       int comma = 0;
-       int status;
-       int i;
-       int n, left, done;
 
-       if (user_msg) *user_msg = '\0';
-       if (output_pairs) *output_pairs = NULL;
+#define USEC 1000000
+static void tv_sub(struct timeval *end, struct timeval *start,
+                  struct timeval *elapsed)
+{
+       elapsed->tv_sec = end->tv_sec - start->tv_sec;
+       if (elapsed->tv_sec > 0) {
+               elapsed->tv_sec--;
+               elapsed->tv_usec = USEC;
+       } else {
+               elapsed->tv_usec = 0;
+       }
+       elapsed->tv_usec += end->tv_usec;
+       elapsed->tv_usec -= start->tv_usec;
 
-       if (strlen(cmd) > (sizeof(mycmd) - 1)) {
-               radlog(L_ERR|L_CONS, "Command line is too long");
-               return -1;
+       if (elapsed->tv_usec >= USEC) {
+               elapsed->tv_usec -= USEC;
+               elapsed->tv_sec++;
        }
+}
+
+
+/** Start a process
+ *
+ * @param cmd Command to execute. This is parsed into argv[] parts,
+ *     then each individual argv part is xlat'ed.
+ * @param request Current reuqest
+ * @param exec_wait set to 1 if you want to read from or write to child
+ * @param[in,out] input_fd pointer to int, receives the stdin file.
+ *     descriptor. Set to NULL and the child will have /dev/null on stdin
+ * @param[in,out] output_fd pinter to int, receives the stdout file
+ *     descriptor. Set to NULL and child will have /dev/null on stdout.
+ * @param input_pairs list of value pairs - these will be put into
+ *     the environment variables of the child.
+ * @param shell_escape values before passing them as arguments.
+ * @return PID of the child process, -1 on error.
+ */
+pid_t radius_start_program(char const *cmd, REQUEST *request, bool exec_wait,
+                          int *input_fd, int *output_fd,
+                          VALUE_PAIR *input_pairs, bool shell_escape)
+{
+#ifndef __MINGW32__
+       VALUE_PAIR *vp;
+       int n;
+       int to_child[2] = {-1, -1};
+       int from_child[2] = {-1, -1};
+       pid_t pid;
+#endif
+       int             argc;
+       int             i;
+       char const      **argv_p;
+       char            *argv[MAX_ARGV], **argv_start = argv;
+       char            argv_buf[4096];
+#define MAX_ENVP 1024
+       char *envp[MAX_ENVP];
+       int envlen = 0;
 
        /*
-        *      Check for bad escapes.
+        *      Stupid array decomposition...
+        *
+        *      If we do memcpy(&argv_p, &argv, sizeof(argv_p)) src ends up being a char **
+        *      pointing to the value of the first element.
         */
-       if (cmd[strlen(cmd) - 1] == '\\') {
-               radlog(L_ERR|L_CONS, "Command line has final backslash, without a following character");
+       memcpy(&argv_p, &argv_start, sizeof(argv_p));
+       argc = rad_expand_xlat(request, cmd, MAX_ARGV, argv_p, true, sizeof(argv_buf), argv_buf);
+       if (argc <= 0) {
+               DEBUG("invalid command line '%s'.", cmd);
                return -1;
        }
 
-       strlcpy(mycmd, cmd, sizeof(mycmd));
 
+#ifndef NDEBUG
+       if (rad_debug_lvl > 2) {
+               DEBUG3("executing cmd %s", cmd);
+               for (i = 0; i < argc; i++) {
+                       DEBUG3("\t[%d] %s", i, argv[i]);
+               }
+       }
+#endif
+
+#ifndef __MINGW32__
        /*
-        *      Split the string into argv's BEFORE doing radius_xlat...
+        *      Open a pipe for child/parent communication, if necessary.
         */
-       from = cmd;
-       to = mycmd;
-       argc = 0;
-       while (*from) {
-               int length;
-
-               /*
-                *      Skip spaces.
-                */
-               if ((*from == ' ') || (*from == '\t')) {
-                       from++;
-                       continue;
+       if (exec_wait) {
+               if (input_fd) {
+                       if (pipe(to_child) != 0) {
+                               DEBUG("Couldn't open pipe to child: %s", fr_syserror(errno));
+                               return -1;
+                       }
                }
+               if (output_fd) {
+                       if (pipe(from_child) != 0) {
+                               DEBUG("Couldn't open pipe from child: %s", fr_syserror(errno));
+                               /* safe because these either need closing or are == -1 */
+                               close(to_child[0]);
+                               close(to_child[1]);
+                               return -1;
+                       }
+               }
+       }
 
-               argv[argc] = to;
-               argc++;
+       envp[0] = NULL;
 
-               if (argc >= (MAX_ARGV - 1)) break;
+       if (input_pairs) {
+               vp_cursor_t cursor;
+               char buffer[1024];
 
                /*
-                *      Copy the argv over to our buffer.
+                *      Set up the environment variables in the
+                *      parent, so we don't call libc functions that
+                *      hold mutexes.  They might be locked when we fork,
+                *      and will remain locked in the child.
                 */
-               while (*from && (*from != ' ') && (*from != '\t')) {
-                       if (to >= mycmd + sizeof(mycmd) - 1) {
-                               return -1; /* ran out of space */
-                       }
+               for (vp = fr_cursor_init(&cursor, &input_pairs);
+                    vp;
+                    vp = fr_cursor_next(&cursor)) {
+                       /*
+                        *      Hmm... maybe we shouldn't pass the
+                        *      user's password in an environment
+                        *      variable...
+                        */
+                       snprintf(buffer, sizeof(buffer), "%s=", vp->da->name);
+                       if (shell_escape) {
+                               char *p;
 
-                       switch (*from) {
-                       case '"':
-                       case '\'':
-                               length = rad_copy_string(to, from);
-                               if (length < 0) {
-                                       radlog(L_ERR|L_CONS, "Invalid string passed as argument for external program");
-                                       return -1;
-                               }
-                               from += length;
-                               to += length;
-                               break;
-
-                       case '%':
-                               if (from[1] == '{') {
-                                       *(to++) = *(from++);
-
-                                       length = rad_copy_variable(to, from);
-                                       if (length < 0) {
-                                               radlog(L_ERR|L_CONS, "Invalid variable expansion passed as argument for external program");
-                                               return -1;
+                               for (p = buffer; *p != '='; p++) {
+                                       if (*p == '-') {
+                                               *p = '_';
+                                       } else if (isalpha((int) *p)) {
+                                               *p = toupper(*p);
                                        }
-                                       from += length;
-                                       to += length;
-                               } else { /* FIXME: catch %%{ ? */
-                                       *(to++) = *(from++);
                                }
-                               break;
-
-                       default:
-                               *(to++) = *(from++);
                        }
-               } /* end of string, or found a space */
-
-               *(to++) = '\0'; /* terminate the string */
-       }
 
-       /*
-        *      We have to have SOMETHING, at least.
-        */
-       if (argc <= 0) {
-               radlog(L_ERR, "Exec-Program: empty command line.");
-               return -1;
-       }
-
-       /*
-        *      Expand each string, as appropriate.
-        */
-       to = argv_buf;
-       left = sizeof(argv_buf);
-       for (i = 0; i < argc; i++) {
-               int sublen;
+                       n = strlen(buffer);
+                       vp_prints_value(buffer + n, sizeof(buffer) - n, vp, shell_escape ? '"' : 0);
 
-               /*
-                *      Don't touch argv's which won't be translated.
-                */
-               if (strchr(argv[i], '%') == NULL) continue;
+                       envp[envlen++] = strdup(buffer);
 
-               sublen = radius_xlat(to, left - 1, argv[i], request, NULL);
-               if (sublen <= 0) {
                        /*
-                        *      Fail to be backwards compatible.
-                        *
-                        *      It's yucky, but it won't break anything,
-                        *      and it won't cause security problems.
+                        *      Don't add too many attributes.
                         */
-                       sublen = 0;
-               }
-
-               argv[i] = to;
-               to += sublen;
-               *(to++) = '\0';
-               left -= sublen;
-               left--;
-
-               if (left <= 0) {
-                       radlog(L_ERR, "Exec-Program: Ran out of space while expanding arguments.");
-                       return -1;
-               }
-       }
-       argv[argc] = NULL;
+                       if (envlen == (MAX_ENVP - 1)) break;
 
-       /*
-        *      Open a pipe for child/parent communication, if necessary.
-        */
-       if (exec_wait) {
-               if (pipe(pd) != 0) {
-                       radlog(L_ERR|L_CONS, "Couldn't open pipe: %s",
-                              strerror(errno));
-                       return -1;
+                       /*
+                        *      NULL terminate for execve
+                        */
+                       envp[envlen] = NULL;
                }
-       } else {
-               /*
-                *      We're not waiting, so we don't look for a
-                *      message, or VP's.
-                */
-               user_msg = NULL;
-               output_pairs = NULL;
        }
 
        if (exec_wait) {
@@ -233,11 +203,7 @@ int radius_exec_program(const char *cmd, REQUEST *request,
        }
 
        if (pid == 0) {
-#define MAX_ENVP 1024
                int devnull;
-               char *envp[MAX_ENVP];
-               int envlen;
-               char buffer[1024];
 
                /*
                 *      Child process.
@@ -251,38 +217,38 @@ int radius_exec_program(const char *cmd, REQUEST *request,
                 */
                devnull = open("/dev/null", O_RDWR);
                if (devnull < 0) {
-                       radlog(L_ERR|L_CONS, "Failed opening /dev/null: %s\n",
-                              strerror(errno));
-                       exit(1);
+                       DEBUG("Failed opening /dev/null: %s\n", fr_syserror(errno));
+
+                       /*
+                        *      Where the status code is interpreted as a module rcode
+                        *      one is subtracted from it, to allow 0 to equal success
+                        *
+                        *      2 is RLM_MODULE_FAIL + 1
+                        */
+                       exit(2);
                }
-               dup2(devnull, STDIN_FILENO);
 
                /*
                 *      Only massage the pipe handles if the parent
                 *      has created them.
                 */
                if (exec_wait) {
-                       /*
-                        *      pd[0] is the FD the child will read from,
-                        *      which we don't want.
-                        */
-                       if (close(pd[0]) != 0) {
-                               radlog(L_ERR|L_CONS, "Can't close pipe: %s",
-                                      strerror(errno));
-                               exit(1);
+                       if (input_fd) {
+                               close(to_child[1]);
+                               dup2(to_child[0], STDIN_FILENO);
+                       } else {
+                               dup2(devnull, STDIN_FILENO);
                        }
 
-                       /*
-                        *      pd[1] is the FD that the child will write to,
-                        *      so we make it STDOUT.
-                        */
-                       if (dup2(pd[1], STDOUT_FILENO) != 1) {
-                               radlog(L_ERR|L_CONS, "Can't dup stdout: %s",
-                                      strerror(errno));
-                               exit(1);
+                       if (output_fd) {
+                               close(from_child[0]);
+                               dup2(from_child[1], STDOUT_FILENO);
+                       } else {
+                               dup2(devnull, STDOUT_FILENO);
                        }
 
                } else {        /* no pipe, STDOUT should be /dev/null */
+                       dup2(devnull, STDIN_FILENO);
                        dup2(devnull, STDOUT_FILENO);
                }
 
@@ -294,7 +260,7 @@ int radius_exec_program(const char *cmd, REQUEST *request,
                 *      If we are debugging, then we want the error
                 *      messages to go to the STDERR of the server.
                 */
-               if (debug_flag == 0) {
+               if (rad_debug_lvl == 0) {
                        dup2(devnull, STDERR_FILENO);
                }
                close(devnull);
@@ -307,83 +273,199 @@ int radius_exec_program(const char *cmd, REQUEST *request,
                closefrom(3);
 
                /*
-                *      Set up the environment variables.
-                *      We're in the child, and it will exit in 4 lines
-                *      anyhow, so memory allocation isn't an issue.
+                *      I swear the signature for execve is wrong and should
+                *      take 'char const * const argv[]'.
+                *
+                *      Note: execve(), unlike system(), treats all the space
+                *      delimited arguments as literals, so there's no need
+                *      to perform additional escaping.
                 */
-               envlen = 0;
-
-               for (vp = input_pairs; vp != NULL; vp = vp->next) {
-                       /*
-                        *      Hmm... maybe we shouldn't pass the
-                        *      user's password in an environment
-                        *      variable...
-                        */
-                       snprintf(buffer, sizeof(buffer), "%s=", vp->name);
-                       if (shell_escape) {
-                               for (p = buffer; *p != '='; p++) {
-                                       if (*p == '-') {
-                                               *p = '_';
-                                       } else if (isalpha((int) *p)) {
-                                               *p = toupper(*p);
-                                       }
-                               }
-                       }
-
-                       n = strlen(buffer);
-                       vp_prints_value(buffer+n, sizeof(buffer) - n, vp, shell_escape);
-
-                       envp[envlen++] = strdup(buffer);
+               execve(argv[0], argv, envp);
+               printf("Failed to execute \"%s\": %s", argv[0], fr_syserror(errno)); /* fork output will be captured */
 
-                       /*
-                        *      Don't add too many attributes.
-                        */
-                       if (envlen == (MAX_ENVP - 1)) break;
-               }
-               envp[envlen] = NULL;
+               /*
+                *      Where the status code is interpreted as a module rcode
+                *      one is subtracted from it, to allow 0 to equal success
+                *
+                *      2 is RLM_MODULE_FAIL + 1
+                */
+               exit(2);
+       }
 
-               execve(argv[0], argv, envp);
-               radlog(L_ERR, "Exec-Program: FAILED to execute %s: %s",
-                      argv[0], strerror(errno));
-               exit(1);
+       /*
+        *      Free child environment variables
+        */
+       for (i = 0; i < envlen; i++) {
+               free(envp[i]);
        }
 
        /*
         *      Parent process.
         */
        if (pid < 0) {
-               radlog(L_ERR|L_CONS, "Couldn't fork %s: %s",
-                      argv[0], strerror(errno));
+               DEBUG("Couldn't fork %s: %s", argv[0], fr_syserror(errno));
+               if (exec_wait) {
+                       /* safe because these either need closing or are == -1 */
+                       close(to_child[0]);
+                       close(to_child[1]);
+                       close(from_child[0]);
+                       close(from_child[1]);
+               }
                return -1;
        }
 
        /*
         *      We're not waiting, exit, and ignore any child's status.
         */
-       if (!exec_wait) {
-               return 0;
+       if (exec_wait) {
+               /*
+                *      Close the ends of the pipe(s) the child is using
+                *      return the ends of the pipe(s) our caller wants
+                *
+                */
+               if (input_fd) {
+                       *input_fd = to_child[1];
+                       close(to_child[0]);
+               }
+               if (output_fd) {
+                       *output_fd = from_child[0];
+                       close(from_child[1]);
+               }
        }
 
-       /*
-        *      Close the FD to which the child writes it's data.
-        *
-        *      If we can't close it, then we close pd[0], and return an
-        *      error.
-        */
-       if (close(pd[1]) != 0) {
-               radlog(L_ERR|L_CONS, "Can't close pipe: %s", strerror(errno));
-               close(pd[0]);
+       return pid;
+#else
+       if (exec_wait) {
+               DEBUG("Wait is not supported");
                return -1;
        }
 
+       {
+               /*
+                *      The _spawn and _exec families of functions are
+                *      found in Windows compiler libraries for
+                *      portability from UNIX. There is a variety of
+                *      functions, including the ability to pass
+                *      either a list or array of parameters, to
+                *      search in the PATH or otherwise, and whether
+                *      or not to pass an environment (a set of
+                *      environment variables). Using _spawn, you can
+                *      also specify whether you want the new process
+                *      to close your program (_P_OVERLAY), to wait
+                *      until the new process is finished (_P_WAIT) or
+                *      for the two to run concurrently (_P_NOWAIT).
+
+                *      _spawn and _exec are useful for instances in
+                *      which you have simple requirements for running
+                *      the program, don't want the overhead of the
+                *      Windows header file, or are interested
+                *      primarily in portability.
+                */
+
+               /*
+                *      FIXME: check return code... what is it?
+                */
+               _spawnve(_P_NOWAIT, argv[0], argv, envp);
+       }
+
+       return 0;
+#endif
+}
+
+/** Read from the child process.
+ *
+ * @param fd file descriptor to read from.
+ * @param pid pid of child, will be reaped if it dies.
+ * @param timeout amount of time to wait, in seconds.
+ * @param answer buffer to write into.
+ * @param left length of buffer.
+ * @return -1 on error, or length of output.
+ */
+int radius_readfrom_program(int fd, pid_t pid, int timeout,
+                           char *answer, int left)
+{
+       int done = 0;
+#ifndef __MINGW32__
+       int status;
+       struct timeval start;
+#ifdef O_NONBLOCK
+       bool nonblock = true;
+#endif
+
+#ifdef O_NONBLOCK
+       /*
+        *      Try to set it non-blocking.
+        */
+       do {
+               int flags;
+
+               if ((flags = fcntl(fd, F_GETFL, NULL)) < 0)  {
+                       nonblock = false;
+                       break;
+               }
+
+               flags |= O_NONBLOCK;
+               if( fcntl(fd, F_SETFL, flags) < 0) {
+                       nonblock = false;
+                       break;
+               }
+       } while (0);
+#endif
+
+
        /*
         *      Read from the pipe until we doesn't get any more or
         *      until the message is full.
         */
-       done = 0;
-       left = sizeof(answer) - 1;
+       gettimeofday(&start, NULL);
        while (1) {
-               status = read(pd[0], answer + done, left);
+               int rcode;
+               fd_set fds;
+               struct timeval when, elapsed, wake;
+
+               FD_ZERO(&fds);
+               FD_SET(fd, &fds);
+
+               gettimeofday(&when, NULL);
+               tv_sub(&when, &start, &elapsed);
+               if (elapsed.tv_sec >= timeout) goto too_long;
+
+               when.tv_sec = timeout;
+               when.tv_usec = 0;
+               tv_sub(&when, &elapsed, &wake);
+
+               rcode = select(fd + 1, &fds, NULL, NULL, &wake);
+               if (rcode == 0) {
+               too_long:
+                       DEBUG("Child PID %u is taking too much time: forcing failure and killing child.", (unsigned int) pid);
+                       kill(pid, SIGTERM);
+                       close(fd); /* should give SIGPIPE to child, too */
+
+                       /*
+                        *      Clean up the child entry.
+                        */
+                       rad_waitpid(pid, &status);
+                       return -1;
+               }
+               if (rcode < 0) {
+                       if (errno == EINTR) continue;
+                       break;
+               }
+
+#ifdef O_NONBLOCK
+               /*
+                *      Read as many bytes as possible.  The kernel
+                *      will return the number of bytes available.
+                */
+               if (nonblock) {
+                       status = read(fd, answer + done, left);
+               } else
+#endif
+                       /*
+                        *      There's at least 1 byte ready: read it.
+                        */
+                       status = read(fd, answer + done, 1);
+
                /*
                 *      Nothing more to read: stop.
                 */
@@ -414,94 +496,155 @@ int radius_exec_program(const char *cmd, REQUEST *request,
                left -= status;
                if (left <= 0) break;
        }
-       answer[done] = 0;
+#endif /* __MINGW32__ */
+
+       /* Strip trailing new lines */
+       while ((done > 0) && (answer[done - 1] == '\n')) {
+               answer[--done] = '\0';
+       }
+
+       return done;
+}
+
+/** Execute a program.
+ *
+ * @param[in,out] ctx to allocate new VALUE_PAIR (s) in.
+ * @param[out] out buffer to append plaintext (non valuepair) output.
+ * @param[in] outlen length of out buffer.
+ * @param[out] output_pairs list of value pairs - child stdout will be parsed and added into this list
+ *     of value pairs.
+ * @param[in] request Current request (may be NULL).
+ * @param[in] cmd Command to execute. This is parsed into argv[] parts, then each individual argv part
+ *     is xlat'ed.
+ * @param[in] input_pairs list of value pairs - these will be available in the environment of the child.
+ * @param[in] exec_wait set to 1 if you want to read from or write to child.
+ * @param[in] shell_escape values before passing them as arguments.
+ * @param[in] timeout amount of time to wait, in seconds.
+
+ * @return 0 if exec_wait==0, exit code if exec_wait!=0, -1 on error.
+ */
+int radius_exec_program(TALLOC_CTX *ctx, char *out, size_t outlen, VALUE_PAIR **output_pairs,
+                       REQUEST *request, char const *cmd, VALUE_PAIR *input_pairs,
+                       bool exec_wait, bool shell_escape, int timeout)
+
+{
+       pid_t pid;
+       int from_child;
+#ifndef __MINGW32__
+       char *p;
+       pid_t child_pid;
+       int comma = 0;
+       int status, ret = 0;
+       ssize_t len;
+       char answer[4096];
+#endif
+
+       RDEBUG2("Executing: %s:", cmd);
+
+       if (out) *out = '\0';
+
+       pid = radius_start_program(cmd, request, exec_wait, NULL, &from_child, input_pairs, shell_escape);
+       if (pid < 0) {
+               return -1;
+       }
+
+       if (!exec_wait) {
+               return 0;
+       }
+
+#ifndef __MINGW32__
+       len = radius_readfrom_program(from_child, pid, timeout, answer, sizeof(answer));
+       if (len < 0) {
+               /*
+                *      Failure - radius_readfrom_program will
+                *      have called close(from_child) for us
+                */
+               RERROR("Failed to read from child output");
+               return -1;
+
+       }
+       answer[len] = '\0';
 
        /*
         *      Make sure that the writer can't block while writing to
         *      a pipe that no one is reading from anymore.
         */
-       close(pd[0]);
+       close(from_child);
 
-       DEBUG2("Exec-Program output: %s", answer);
+       if (len == 0) {
+               goto wait;
+       }
 
        /*
         *      Parse the output, if any.
         */
-       if (done) {
-               n = T_OP_INVALID;
-               if (output_pairs) {
-                       /*
-                        *      For backwards compatibility, first check
-                        *      for plain text (user_msg).
-                        */
-                       vp = NULL;
-                       n = userparse(answer, &vp);
-                       if (vp) {
-                               pairfree(&vp);
-                       }
-               }
-
-               if (n == T_OP_INVALID) {
-                       DEBUG("Exec-Program-Wait: plaintext: %s", answer);
-                       if (user_msg) {
-                               strlcpy(user_msg, answer, msg_len);
+       if (output_pairs) {
+               /*
+                *      HACK: Replace '\n' with ',' so that
+                *      fr_pair_list_afrom_str() can parse the buffer in
+                *      one go (the proper way would be to
+                *      fix fr_pair_list_afrom_str(), but oh well).
+                */
+               for (p = answer; *p; p++) {
+                       if (*p == '\n') {
+                               *p = comma ? ' ' : ',';
+                               p++;
+                               comma = 0;
                        }
-               } else {
-                       /*
-                        *      HACK: Replace '\n' with ',' so that
-                        *      userparse() can parse the buffer in
-                        *      one go (the proper way would be to
-                        *      fix userparse(), but oh well).
-                        */
-                       for (p = answer; *p; p++) {
-                               if (*p == '\n') {
-                                       *p = comma ? ' ' : ',';
-                                       p++;
-                                       comma = 0;
-                               }
-                               if (*p == ',') comma++;
+                       if (*p == ',') {
+                               comma++;
                        }
+               }
 
-                       /*
-                        *      Replace any trailing comma by a NUL.
-                        */
-                       if (answer[strlen(answer) - 1] == ',') {
-                               answer[strlen(answer) - 1] = '\0';
-                       }
+               /*
+                *      Replace any trailing comma by a NUL.
+                */
+               if (answer[len - 1] == ',') {
+                       answer[--len] = '\0';
+               }
 
-                       radlog(L_DBG,"Exec-Program-Wait: value-pairs: %s", answer);
-                       if (userparse(answer, &vp) == T_OP_INVALID) {
-                               radlog(L_ERR, "Exec-Program-Wait: %s: unparsable reply", cmd);
+               if (fr_pair_list_afrom_str(ctx, answer, output_pairs) == T_INVALID) {
+                       RERROR("Failed parsing output from: %s: %s", cmd, fr_strerror());
+                       strlcpy(out, answer, len);
+                       ret = -1;
+               }
+       /*
+        *      We've not been told to extract output pairs,
+        *      just copy the programs output to the out
+        *      buffer.
+        */
 
-                       } else {
-                               /*
-                                *      Tell the caller about the value
-                                *      pairs.
-                                */
-                               *output_pairs = vp;
-                       }
-               } /* else the answer was a set of VP's, not a text message */
-       } /* else we didn't read anything from the child */
+       } else if (out) {
+               strlcpy(out, answer, outlen);
+       }
 
        /*
         *      Call rad_waitpid (should map to waitpid on non-threaded
         *      or single-server systems).
         */
+wait:
        child_pid = rad_waitpid(pid, &status);
        if (child_pid == 0) {
-               radlog(L_DBG, "Exec-Program: Timeout waiting for child");
-               return 2;
+               RERROR("Timeout waiting for child");
+
+               return -2;
        }
 
        if (child_pid == pid) {
                if (WIFEXITED(status)) {
                        status = WEXITSTATUS(status);
-                       radlog(L_DBG, "Exec-Program: returned: %d", status);
-                       return status;
+                       if ((status != 0) || (ret < 0)) {
+                               RERROR("Program returned code (%d) and output '%s'", status, answer);
+                       } else {
+                               RDEBUG2("Program returned code (%d) and output '%s'", status, answer);
+                       }
+
+                       return ret < 0 ? ret : status;
                }
        }
 
-       radlog(L_ERR|L_CONS, "Exec-Program: Abnormal child exit: %s",
-              strerror(errno));
-       return 1;
+       RERROR("Abnormal child exit: %s", fr_syserror(errno));
+#endif /* __MINGW32__ */
+
+       return -1;
 }