Merge branch 'master' of git://git.fedorahosted.org/git/abrt

14 files changed, 269 insertions, 71 deletions

diff --git a/lib/Utils/CommLayerInner.cpp b/lib/Utils/CommLayerInner.cpp
index 133e97d..4a3b80a 100644
--- a/lib/Utils/CommLayerInner.cpp
+++ b/lib/Utils/CommLayerInner.cpp
@@ -68,11 +68,9 @@ void update_client(const char *fmt, ...)
 
     va_list p;
     va_start(p, fmt);
-    char *msg;
-    int used = vasprintf(&msg, fmt, p);
+    char *msg = xvasprintf(fmt, p);
     va_end(p);
-    if (used < 0)
-        return;
 
     s_pObs->Status(msg, peer, key);
+    free(msg);
 }
diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp
index b4c3ee4..a0a52ab 100644
--- a/lib/Utils/DebugDump.cpp
+++ b/lib/Utils/DebugDump.cpp
@@ -65,7 +65,9 @@ CDebugDump::CDebugDump() :
     m_sDebugDumpDir(""),
     m_pGetNextFileDir(NULL),
     m_bOpened(false),
-    m_bLocked(false)
+    m_bLocked(false),
+    m_uid(0),
+    m_gid(0)
 {}
 
 CDebugDump::~CDebugDump()
@@ -97,6 +99,15 @@ void CDebugDump::Open(const char *pDir)
     }
     Lock();
     m_bOpened = true;
+    /* In case caller would want to create more files, he'll need uid:gid */
+    m_uid = 0;
+    m_gid = 0;
+    struct stat stat_buf;
+    if (stat(m_sDebugDumpDir.c_str(), &stat_buf) == 0)
+    {
+        m_uid = stat_buf.st_uid;
+        m_gid = stat_buf.st_gid;
+    }
 }
 
 bool CDebugDump::Exist(const char* pPath)
@@ -115,7 +126,15 @@ static bool GetAndSetLock(const char* pLockFile, const char* pPID)
         char pid_buf[sizeof(pid_t)*3 + 4];
         ssize_t r = readlink(pLockFile, pid_buf, sizeof(pid_buf) - 1);
         if (r < 0)
+        {
+            if (errno == ENOENT)
+            {
+                /* Looks like pLockFile was deleted */
+                usleep(10 * 1000); /* avoid CPU eating loop */
+                continue;
+            }
             perror_msg_and_die("Can't read lock file '%s'", pLockFile);
+        }
         pid_buf[r] = '\0';
 
         if (strcmp(pid_buf, pPID) == 0)
@@ -209,8 +228,8 @@ void CDebugDump::Lock()
         error_msg_and_die("Locking bug on '%s'", m_sDebugDumpDir.c_str());
 
     std::string lockFile = m_sDebugDumpDir + ".lock";
-    char pid_buf[sizeof(int)*3 + 2];
-    sprintf(pid_buf, "%u", (unsigned)getpid());
+    char pid_buf[sizeof(long)*3 + 2];
+    sprintf(pid_buf, "%lu", (long)getpid());
     while ((m_bLocked = GetAndSetLock(lockFile.c_str(), pid_buf)) != true)
     {
         sleep(1); /* was 0.5 seconds */
@@ -228,7 +247,26 @@ void CDebugDump::UnLock()
     }
 }
 
-void CDebugDump::Create(const char *pDir, int64_t uid)
+/* Create a fresh empty debug dump dir.
+ *
+ * Security: we should not allow users to write new files or write
+ * into existing ones, but they should be able to read them.
+ * 
+ * @param uid
+ *   Crashed application's User Id
+ *
+ * We currently have only three callers:
+ * kernel oops hook: uid=0
+ *  this hook runs under 0:0
+ * ccpp hook: uid=uid of crashed user's binary
+ *  this hook runs under 0:0
+ * python hook: uid=uid of crashed user's script
+ *  this hook runs under abrt:gid
+ *
+ * Currently, we set dir's gid to passwd(uid)->pw_gid parameter, and we set uid to
+ * abrt's user id. We do not allow write access to group.
+ */
+void CDebugDump::Create(const char *pDir, uid_t uid)
 {
     if (m_bOpened)
     {
@@ -244,25 +282,45 @@ void CDebugDump::Create(const char *pDir, int64_t uid)
     Lock();
     m_bOpened = true;
 
-    if (mkdir(m_sDebugDumpDir.c_str(), 0700) == -1)
+    /* Was creating it with mode 0700 and user as the owner, but this allows 
+     * the user to replace any file in the directory, changing security-sensitive data
+     * (e.g. "uid", "analyzer", "executable")
+     */
+    if (mkdir(m_sDebugDumpDir.c_str(), 0750) == -1)
     {
         UnLock();
         m_bOpened = false;
         throw CABRTException(EXCEP_DD_OPEN, "Can't create dir '%s'", pDir);
     }
-    if (chmod(m_sDebugDumpDir.c_str(), 0700) == -1)
+
+    /* mkdir's mode (above) can be affected by umask, fix it */
+    if (chmod(m_sDebugDumpDir.c_str(), 0750) == -1)
     {
         UnLock();
         m_bOpened = false;
         throw CABRTException(EXCEP_DD_OPEN, "Can't change mode of '%s'", pDir);
     }
-    struct passwd* pw = getpwuid(uid);
-    gid_t gid = pw ? pw->pw_gid : uid;
-    if (chown(m_sDebugDumpDir.c_str(), uid, gid) == -1)
+
+    /* Get ABRT's user id */
+    m_uid = 0; 
+    struct passwd *pw = getpwnam("abrt");
+    if (pw)
+        m_uid = pw->pw_uid;
+    else
+        error_msg("User 'abrt' does not exist, using uid 0");
+
+    /* Get crashed application's group id */
+    m_gid = 0;
+    pw = getpwuid(uid);
+    if (pw)
+        m_gid = pw->pw_gid;
+    else
+      error_msg("User %lu does not exist, using gid 0", (long)uid);
+
+    if (chown(m_sDebugDumpDir.c_str(), m_uid, m_gid) == -1)
     {
-        /* if /var/cache/abrt is writable by all, _aborting_ here is not useful */
-        /* let's just warn */
-        perror_msg("can't change '%s' ownership to %u:%u", m_sDebugDumpDir.c_str(), (int)uid, (int)gid);
+        perror_msg("can't change '%s' ownership to %lu:%lu", m_sDebugDumpDir.c_str(), 
+		   (long)m_uid, (long)m_gid);
     }
 
     SaveText(FILENAME_UID, to_string(uid).c_str());
@@ -294,7 +352,7 @@ static void DeleteFileDir(const char *pDir)
         }
     }
     closedir(dir);
-    if (remove(pDir) == -1)
+    if (rmdir(pDir) == -1)
     {
         throw CABRTException(EXCEP_DD_DELETE, "Can't remove dir %s", pDir);
     }
@@ -359,12 +417,18 @@ static void LoadTextFile(const char *pPath, std::string& pData)
     fclose(fp);
 }
 
-static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize)
+static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize, uid_t uid, gid_t gid)
 {
-    int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0666);
+    /* "Why 0640?!" See ::Create() for security analysis */
+    unlink(pPath);
+    int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0640);
     if (fd < 0)
     {
-        throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s'", pPath);
+        throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s': %s", pPath, errno ? strerror(errno) : "errno == 0");
+    }
+    if (fchown(fd, uid, gid) == -1)
+    {
+        perror_msg("can't change '%s' ownership to %lu:%lu", pPath, (long)uid, (long)gid);
     }
     unsigned r = full_write(fd, pData, pSize);
     close(fd);
@@ -391,8 +455,9 @@ void CDebugDump::SaveText(const char* pName, const char* pData)
         throw CABRTException(EXCEP_DD_OPEN, "DebugDump is not opened");
     }
     std::string fullPath = concat_path_file(m_sDebugDumpDir.c_str(), pName);
-    SaveBinaryFile(fullPath.c_str(), pData, strlen(pData));
+    SaveBinaryFile(fullPath.c_str(), pData, strlen(pData), m_uid, m_gid);
 }
+
 void CDebugDump::SaveBinary(const char* pName, const char* pData, unsigned pSize)
 {
     if (!m_bOpened)
@@ -400,7 +465,7 @@ void CDebugDump::SaveBinary(const char* pName, const char* pData, unsigned pSize
         throw CABRTException(EXCEP_DD_OPEN, "DebugDump is not opened");
     }
     std::string fullPath = concat_path_file(m_sDebugDumpDir.c_str(), pName);
-    SaveBinaryFile(fullPath.c_str(), pData, pSize);
+    SaveBinaryFile(fullPath.c_str(), pData, pSize, m_uid, m_gid);
 }
 
 void CDebugDump::InitGetNextFile()
diff --git a/lib/Utils/DebugDump.h b/lib/Utils/DebugDump.h
index c59552e..d3eebb4 100644
--- a/lib/Utils/DebugDump.h
+++ b/lib/Utils/DebugDump.h
@@ -51,6 +51,8 @@ class CDebugDump
         DIR* m_pGetNextFileDir;
         bool m_bOpened;
         bool m_bLocked;
+        uid_t m_uid;
+        gid_t m_gid;
 
         void SaveKernelArchitectureRelease();
 
@@ -62,7 +64,7 @@ class CDebugDump
         ~CDebugDump();
 
         void Open(const char *pDir);
-        void Create(const char *pDir, int64_t uid);
+        void Create(const char *pDir, uid_t uid);
         void Delete();
         void Close();
 
diff --git a/lib/Utils/Makefile.am b/lib/Utils/Makefile.am
index d5e9d98..a944d97 100644
--- a/lib/Utils/Makefile.am
+++ b/lib/Utils/Makefile.am
@@ -16,7 +16,7 @@ libABRTUtils_la_SOURCES = \
     daemon.cpp \
     skip_whitespace.cpp \
     xatonum.cpp \
-    popen_and_save_output.cpp \
+    spawn.cpp \
     stringops.cpp \
     dirsize.cpp \
     DebugDump.h DebugDump.cpp \
diff --git a/lib/Utils/Polkit.cpp b/lib/Utils/Polkit.cpp
index c868e1a..a7e4a5d 100644
--- a/lib/Utils/Polkit.cpp
+++ b/lib/Utils/Polkit.cpp
@@ -46,14 +46,12 @@ static PolkitResult do_check(PolkitSubject *subject, const char *action_id)
     GCancellable * cancellable;
 
     authority = polkit_authority_get();
-
     cancellable = g_cancellable_new();
 
-    g_timeout_add(POLKIT_TIMEOUT * 1000,
+    guint cancel_timeout = g_timeout_add(POLKIT_TIMEOUT * 1000,
                    (GSourceFunc) do_cancel,
                    cancellable);
 
-
     result = polkit_authority_check_authorization_sync(authority,
                 subject,
                 action_id,
@@ -61,7 +59,8 @@ static PolkitResult do_check(PolkitSubject *subject, const char *action_id)
                 POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION,
                 cancellable,
                 &error);
-
+    g_object_unref(authority);
+    g_source_remove(cancel_timeout);
     if (error)
     {
         g_error_free(error);
@@ -71,11 +70,18 @@ static PolkitResult do_check(PolkitSubject *subject, const char *action_id)
     if (result)
     {
         if (polkit_authorization_result_get_is_challenge(result))
+        {
             /* Can't happen (happens only with
              * POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE flag) */
+            g_object_unref(result);
             return PolkitChallenge;
+        }
         if (polkit_authorization_result_get_is_authorized(result))
+        {
+            g_object_unref(result);
             return PolkitYes;
+        }
+        g_object_unref(result);
         return PolkitNo;
     }
 
diff --git a/lib/Utils/abrt_dbus.h b/lib/Utils/abrt_dbus.h
index bc9889a..25e099e 100644
--- a/lib/Utils/abrt_dbus.h
+++ b/lib/Utils/abrt_dbus.h
@@ -154,6 +154,9 @@ enum {
     ABRT_DBUS_ERROR = -1,
     ABRT_DBUS_LAST_FIELD = 0,
     ABRT_DBUS_MORE_FIELDS = 1,
+    /* note that dbus_message_iter_next() returns FALSE on last field
+     * and TRUE if there are more fields.
+     * It maps exactly on the above constants. */
 };
 /* Checks type, loads data, advances to the next arg.
  * Returns TRUE if next arg exists.
diff --git a/lib/Utils/abrt_xmlrpc.cpp b/lib/Utils/abrt_xmlrpc.cpp
index ae1d098..e3a4648 100644
--- a/lib/Utils/abrt_xmlrpc.cpp
+++ b/lib/Utils/abrt_xmlrpc.cpp
@@ -15,6 +15,16 @@ CURL* xcurl_easy_init()
     return curl;
 }
 
+#if 1
+void throw_xml_fault(xmlrpc_env *env)
+{
+    std::string errmsg = ssprintf("XML-RPC Fault: %s(%d)", env->fault_string, env->fault_code);
+    xmlrpc_env_clean(env); // this is needed ONLY if fault_occurred
+    xmlrpc_env_init(env); // just in case user catches ex and _continues_ to use env
+    error_msg("%s", errmsg.c_str()); // show error in daemon log
+    throw CABRTException(EXCEP_PLUGIN, errmsg.c_str());
+}
+#else
 void throw_if_xml_fault_occurred(xmlrpc_env *env)
 {
     if (env->fault_occurred)
@@ -26,6 +36,7 @@ void throw_if_xml_fault_occurred(xmlrpc_env *env)
         throw CABRTException(EXCEP_PLUGIN, errmsg.c_str());
     }
 }
+#endif
 
 void abrt_xmlrpc_conn::new_xmlrpc_client(const char* url, bool no_ssl_verify)
 {
@@ -60,15 +71,16 @@ void abrt_xmlrpc_conn::new_xmlrpc_client(const char* url, bool no_ssl_verify)
                         PACKAGE_NAME, VERSION,
                         &clientParms, XMLRPC_CPSIZE(transportparm_size),
                         &m_pClient);
-    throw_if_xml_fault_occurred(&env);
+    if (env.fault_occurred)
+        throw_xml_fault(&env);
 
     m_pServer_info = xmlrpc_server_info_new(&env, url);
     if (env.fault_occurred)
     {
         xmlrpc_client_destroy(m_pClient);
         m_pClient = NULL;
+        throw_xml_fault(&env);
     }
-    throw_if_xml_fault_occurred(&env);
 }
 
 void abrt_xmlrpc_conn::destroy_xmlrpc_client()
diff --git a/lib/Utils/abrt_xmlrpc.h b/lib/Utils/abrt_xmlrpc.h
index 352e80c..a8bd2cc 100644
--- a/lib/Utils/abrt_xmlrpc.h
+++ b/lib/Utils/abrt_xmlrpc.h
@@ -24,7 +24,7 @@ struct abrt_xmlrpc_conn {
 };
 
 /* Utility functions */
-void throw_if_xml_fault_occurred(xmlrpc_env *env);
+void throw_xml_fault(xmlrpc_env *env);
 CURL* xcurl_easy_init();
 
 #endif
diff --git a/lib/Utils/copyfd.cpp b/lib/Utils/copyfd.cpp
index 9abe752..fdc568a 100644
--- a/lib/Utils/copyfd.cpp
+++ b/lib/Utils/copyfd.cpp
@@ -106,7 +106,7 @@ off_t copyfd_eof(int fd1, int fd2)
 	return full_fd_action(fd1, fd2, 0);
 }
 
-off_t copy_file(const char *src_name, const char *dst_name)
+off_t copy_file(const char *src_name, const char *dst_name, int mode)
 {
     off_t r;
     int src = open(src_name, O_RDONLY);
@@ -115,7 +115,7 @@ off_t copy_file(const char *src_name, const char *dst_name)
         perror_msg("Can't open '%s'", src_name);
         return -1;
     }
-    int dst = open(dst_name, O_WRONLY | O_TRUNC | O_CREAT, 0666);
+    int dst = open(dst_name, O_WRONLY | O_TRUNC | O_CREAT, mode);
     if (dst < 0)
     {
         close(src);
diff --git a/lib/Utils/daemon.cpp b/lib/Utils/daemon.cpp
index 7aa891c..0527062 100644
--- a/lib/Utils/daemon.cpp
+++ b/lib/Utils/daemon.cpp
@@ -78,12 +78,12 @@ static char *append_escaped(char *start, const char *s)
 #define COMMAND_LINE_SIZE 2048
 char* get_cmdline(pid_t pid)
 {
-    char path[sizeof("/proc/%u/cmdline") + sizeof(int)*3];
+    char path[sizeof("/proc/%lu/cmdline") + sizeof(long)*3];
     char cmdline[COMMAND_LINE_SIZE];
     char escaped_cmdline[COMMAND_LINE_SIZE*4 + 4];
 
     escaped_cmdline[1] = '\0';
-    sprintf(path, "/proc/%u/cmdline", (int)pid);
+    sprintf(path, "/proc/%lu/cmdline", (long)pid);
     int fd = open(path, O_RDONLY);
     if (fd >= 0)
     {
diff --git a/lib/Utils/popen_and_save_output.cpp b/lib/Utils/popen_and_save_output.cpp
deleted file mode 100644
index 4bcbcac..0000000
--- a/lib/Utils/popen_and_save_output.cpp
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * Utility routines.
- *
- * Licensed under GPLv2 or later, see file COPYING in this tarball for details.
- */
-#include "abrtlib.h"
-
-using namespace std;
-
-string popen_and_save_output(const char *cmd)
-{
-    string result;
-
-    FILE *fp = popen(cmd, "r");
-    if (fp == NULL) /* fork or pipe failed; or out-of-mem */
-    {
-        return result;
-    }
-
-    size_t sz;
-    char buf[BUFSIZ + 1];
-    while ((sz = fread(buf, 1, sizeof(buf)-1, fp)) > 0)
-    {
-        buf[sz] = '\0';
-        result += buf;
-    }
-    pclose(fp);
-
-    return result;
-}
diff --git a/lib/Utils/spawn.cpp b/lib/Utils/spawn.cpp
new file mode 100644
index 0000000..d3e6ac6
--- /dev/null
+++ b/lib/Utils/spawn.cpp
@@ -0,0 +1,136 @@
+/*
+ * Utility routines.
+ *
+ * Licensed under GPLv2, see file COPYING in this tarball for details.
+ */
+#include "abrtlib.h"
+
+using namespace std;
+
+static string concat_str_vector(char **strings)
+{
+	string result;
+	while (*strings) {
+	        result += *strings++;
+		if (*strings)
+			result += ' ';
+	}
+	return result;
+}
+
+/* Returns pid */
+pid_t fork_execv_on_steroids(int flags,
+		char **argv,
+		int *pipefds,
+		char **unsetenv_vec,
+		const char *dir,
+		uid_t uid)
+{
+	pid_t child;
+	/* Reminder: [0] is read end, [1] is write end */
+	int pipe_to_child[2];
+	int pipe_fm_child[2];
+
+	/* Sanitize flags */
+	if (!pipefds)
+		flags &= ~(EXECFLG_INPUT | EXECFLG_OUTPUT);
+
+	if (flags & EXECFLG_INPUT)
+		xpipe(pipe_to_child);
+	if (flags & EXECFLG_OUTPUT)
+		xpipe(pipe_fm_child);
+
+	fflush(NULL);
+	child = fork();
+	if (child == -1) {
+		perror_msg_and_die("fork");
+	}
+	if (child == 0) {
+		/* Child */
+
+		/* Play with stdio descriptors */
+		if (flags & EXECFLG_INPUT) {
+			xmove_fd(pipe_to_child[0], STDIN_FILENO);
+		} else if (flags & EXECFLG_INPUT_NUL) {
+			xmove_fd(xopen("/dev/null", O_RDWR), STDIN_FILENO);
+		}
+		if (flags & EXECFLG_OUTPUT) {
+			xmove_fd(pipe_fm_child[1], STDOUT_FILENO);
+		} else if (flags & EXECFLG_OUTPUT_NUL) {
+			xmove_fd(xopen("/dev/null", O_RDWR), STDOUT_FILENO);
+		}
+		if (flags & EXECFLG_ERR2OUT) {
+			/* Want parent to see errors in the same stream */
+			xdup2(STDOUT_FILENO, STDERR_FILENO);
+		} else if (flags & EXECFLG_ERR_NUL) {
+			xmove_fd(xopen("/dev/null", O_RDWR), STDERR_FILENO);
+		}
+
+		if (flags & EXECFLG_SETGUID) {
+			struct passwd* pw = getpwuid(uid);
+			gid_t gid = pw ? pw->pw_gid : uid;
+			setgroups(1, &gid);
+			xsetregid(gid, gid);
+			xsetreuid(uid, uid);
+		}
+		if (flags & EXECFLG_SETSID)
+			setsid();
+
+		if (unsetenv_vec) {
+			while (*unsetenv_vec)
+				unsetenv(*unsetenv_vec++);
+		}
+
+		if (dir)
+			xchdir(dir);
+
+		VERB1 log("Executing: %s", concat_str_vector(argv).c_str());
+		execvp(argv[0], argv);
+		if (!(flags & EXECFLG_QUIET))
+			perror_msg("Can't execute '%s'", argv[0]);
+		exit(127); /* shell uses this exitcode in this case */
+	}
+
+	if (flags & EXECFLG_INPUT) {
+		close(pipe_to_child[0]);
+		pipefds[1] = pipe_to_child[1];
+	}
+	if (flags & EXECFLG_OUTPUT) {
+		close(pipe_fm_child[1]);
+		pipefds[0] = pipe_fm_child[0];
+	}
+
+	return child;
+}
+
+char *run_in_shell_and_save_output(int flags,
+		const char *cmd,
+		const char *dir,
+		size_t *size_p)
+{
+	flags |= EXECFLG_OUTPUT;
+	flags &= ~EXECFLG_INPUT;
+
+	const char *argv[] = { "/bin/sh", "sh", "-c", cmd, NULL };
+	int pipeout[2];
+	pid_t child = fork_execv_on_steroids(flags, (char **)argv, pipeout,
+		/*unsetenv_vec:*/ NULL, dir, /*uid (unused):*/ 0);
+
+	size_t pos = 0;
+	char *result = NULL;
+	while (1) {
+		result = (char*) xrealloc(result, pos + 4*1024 + 1);
+		size_t sz = safe_read(pipeout[0], result + pos, 4*1024);
+		if (sz <= 0) {
+			break;
+		}
+		pos += sz;
+	}
+	result[pos] = '\0';
+	if (size_p)
+		*size_p = pos;
+	close(pipeout[0]);
+	waitpid(child, NULL, 0);
+
+	return result;
+}
diff --git a/lib/Utils/xatonum.cpp b/lib/Utils/xatonum.cpp
index b096ca8..8314629 100644
--- a/lib/Utils/xatonum.cpp
+++ b/lib/Utils/xatonum.cpp
@@ -9,7 +9,7 @@
 
 unsigned xatou(const char *numstr)
 {
-	unsigned r;
+	unsigned long r;
 	int old_errno;
 	char *e;
 
@@ -19,7 +19,7 @@ unsigned xatou(const char *numstr)
 	old_errno = errno;
 	errno = 0;
 	r = strtoul(numstr, &e, 10);
-	if (errno || numstr == e || *e != '\0')
+	if (errno || numstr == e || *e != '\0' || r > UINT_MAX)
 		goto inval; /* error / no digits / illegal trailing chars */
 	errno = old_errno; /* Ok.  So restore errno. */
 	return r;
diff --git a/lib/Utils/xfuncs.cpp b/lib/Utils/xfuncs.cpp
index 8621b5f..f360d33 100644
--- a/lib/Utils/xfuncs.cpp
+++ b/lib/Utils/xfuncs.cpp
@@ -141,6 +141,12 @@ off_t xlseek(int fd, off_t offset, int whence)
 	return off;
 }
 
+void xchdir(const char *path)
+{
+	if (chdir(path))
+		perror_msg_and_die("chdir(%s)", path);
+}
+
 char* xvasprintf(const char *format, va_list p)
 {
 	int r;
@@ -152,8 +158,8 @@ char* xvasprintf(const char *format, va_list p)
 #else
 	// Bloat for systems that haven't got the GNU extension.
 	va_list p2;
-	r = vsnprintf(NULL, 0, format, p);
 	va_copy(p2, p);
+	r = vsnprintf(NULL, 0, format, p);
 	string_ptr = xmalloc(r+1);
 	r = vsnprintf(string_ptr, r+1, format, p2);
 	va_end(p2);
@@ -365,13 +371,13 @@ bool string_to_bool(const char *s)
 void xsetreuid(uid_t ruid, uid_t euid)
 {
 	if (setreuid(ruid, euid) != 0)
-		perror_msg_and_die("can't set %cid %d", 'u', (int)ruid);
+		perror_msg_and_die("can't set %cid %lu", 'u', (long)ruid);
 }
 
-void xsetregid(gid_t rgid, uid_t egid)
+void xsetregid(gid_t rgid, gid_t egid)
 {
 	if (setregid(rgid, egid) != 0)
-		perror_msg_and_die("can't set %cid %d", 'g', (int)rgid);
+		perror_msg_and_die("can't set %cid %lu", 'g', (long)rgid);
 }
 
 uid_t getuidbyname(const char* login)