Refactor xrdp_listen to allow for privilege drop

- xrdp_listen.c is refactored so we can create the
  listening socket(s) before dropping privileges.
- The code which reads startup params from xrdp.ini
  is moved from xrdp_listen.c to xrdp.c, so it
  is only called once if we test the listen before
  starting the daemon.

common/thread_calls.c

@@ -122,8 +122,11 @@ tc_mutex_delete(tbus mutex)
     pthread_mutex_t *lmutex;
 
     lmutex = (pthread_mutex_t *)mutex;
-    pthread_mutex_destroy(lmutex);
+    if (lmutex != NULL)
-    g_free(lmutex);
+    {
+        pthread_mutex_destroy(lmutex);
+        g_free(lmutex);
+    }
 #endif
 }
 

xrdp/xrdp.c

@@ -240,6 +240,127 @@ xrdp_process_params(int argc, char **argv,
     return 0;
 }
 
+/*****************************************************************************/
+/**
+ *
+ * @brief  Read additional startup parameters from xrdp.ini
+ *
+ * @param  [in,out] startup parameters from the command line
+ * @return 0 on success
+ *
+ */
+static int
+read_xrdp_ini_startup_params(struct xrdp_startup_params *startup_params)
+{
+    int rv = 0;
+    int fd;
+    int index;
+    int port_override;
+    int fork_override;
+    const char *name;
+    const char *val;
+    struct list *names;
+    struct list *values;
+
+    port_override = startup_params->port[0] != 0;
+    fork_override = startup_params->fork;
+    names = list_create();
+    names->auto_free = 1;
+    values = list_create();
+    values->auto_free = 1;
+
+    fd = g_file_open_ro(startup_params->xrdp_ini);
+    if (fd < 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Can't open %s [%s]", startup_params->xrdp_ini,
+            g_get_strerror());
+        rv = 1;
+    }
+    else if (file_read_section(fd, "globals", names, values) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Can't read [Globals] from %s",
+            startup_params->xrdp_ini);
+        rv = 1;
+    }
+    else
+    {
+        for (index = 0; index < names->count; index++)
+        {
+            name = (const char *)list_get_item(names, index);
+            val = (const char *)list_get_item(values, index);
+            if (name == 0 || val == 0)
+            {
+                continue;
+            }
+
+            if (g_strcasecmp(name, "port") == 0)
+            {
+                if (port_override == 0)
+                {
+                    g_strncpy(startup_params->port, val,
+                              sizeof(startup_params->port) - 1);
+                }
+            }
+
+            else if (g_strcasecmp(name, "fork") == 0)
+            {
+                if (fork_override == 0)
+                {
+                    startup_params->fork = g_text2bool(val);
+                }
+            }
+
+            else if (g_strcasecmp(name, "tcp_nodelay") == 0)
+            {
+                startup_params->tcp_nodelay = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_keepalive") == 0)
+            {
+                startup_params->tcp_keepalive = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0)
+            {
+                startup_params->tcp_send_buffer_bytes = g_atoi(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0)
+            {
+                startup_params->tcp_recv_buffer_bytes = g_atoi(val);
+            }
+
+            else if (g_strcasecmp(name, "use_vsock") == 0)
+            {
+                startup_params->use_vsock = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "runtime_user") == 0)
+            {
+                g_snprintf(startup_params->runtime_user,
+                           sizeof(startup_params->runtime_user),
+                           "%s", val);
+            }
+
+            else if (g_strcasecmp(name, "runtime_group") == 0)
+            {
+                g_snprintf(startup_params->runtime_group,
+                           sizeof(startup_params->runtime_group),
+                           "%s", val);
+            }
+        }
+    }
+
+    list_delete(names);
+    list_delete(values);
+    if (fd >= 0)
+    {
+        g_file_close(fd);
+    }
+    return rv;
+}
+
+
 /*****************************************************************************/
 /* Basic sanity checks before any forking */
 static int
@@ -299,7 +420,7 @@ xrdp_sanity_check(void)
 int
 main(int argc, char **argv)
 {
-    int exit_status = 0;
+    int exit_status = 1;
     enum logReturns error;
     struct xrdp_startup_params startup_params = {0};
     int pid;
@@ -428,6 +549,13 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
+    if (!read_xrdp_ini_startup_params(&startup_params))
+    {
+        log_end();
+        g_deinit();
+        g_exit(1);
+    }
+
     if (g_file_exist(pid_file)) /* xrdp.pid */
     {
         LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running.");
@@ -473,8 +601,14 @@ main(int argc, char **argv)
 
     if (daemon)
     {
-        /* if can't listen, exit with failure status */
+        /* Before daemonising, check we can listen.
-        if (xrdp_listen_test(&startup_params) != 0)
+         * If we can't listen, exit with failure status */
+        struct xrdp_listen *xrdp_listen;
+        xrdp_listen = xrdp_listen_create(&startup_params);
+        int status = xrdp_listen_init(xrdp_listen);
+        xrdp_listen_delete(xrdp_listen);
+
+        if (status != 0)
         {
             LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, "
                 "possibly address already in use.");
@@ -540,43 +674,51 @@ main(int argc, char **argv)
         /* end of daemonizing code */
     }
 
-    g_set_threadid(tc_get_threadid());
+    g_listen = xrdp_listen_create(&startup_params);
-    g_listen = xrdp_listen_create();
+    if (xrdp_listen_init(g_listen) != 0)
-    g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */
-    g_signal_pipe(xrdp_sig_no_op);          /* SIGPIPE */
-    g_signal_terminate(xrdp_shutdown);      /* SIGTERM */
-    g_signal_child_stop(xrdp_child);        /* SIGCHLD */
-    g_signal_hang_up(xrdp_sig_no_op);       /* SIGHUP */
-    g_set_sync_mutex(tc_mutex_create());
-    g_set_sync1_mutex(tc_mutex_create());
-    pid = g_getpid();
-    LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid);
-    g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid);
-    g_set_term_event(g_create_wait_obj(text));
-
-    if (g_get_term() == 0)
     {
-        LOG(LOG_LEVEL_WARNING, "error creating g_term_event");
+        LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, "
+            "possibly address already in use.");
+    }
+    else
+    {
+        g_set_threadid(tc_get_threadid());
+        g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */
+        g_signal_pipe(xrdp_sig_no_op);          /* SIGPIPE */
+        g_signal_terminate(xrdp_shutdown);      /* SIGTERM */
+        g_signal_child_stop(xrdp_child);        /* SIGCHLD */
+        g_signal_hang_up(xrdp_sig_no_op);       /* SIGHUP */
+        g_set_sync_mutex(tc_mutex_create());
+        g_set_sync1_mutex(tc_mutex_create());
+        pid = g_getpid();
+        LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid);
+        g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid);
+        g_set_term_event(g_create_wait_obj(text));
+
+        if (g_get_term() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_term_event");
+        }
+
+        g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid);
+        g_set_sigchld_event(g_create_wait_obj(text));
+
+        if (g_get_sigchld() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event");
+        }
+
+        g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid);
+        g_set_sync_event(g_create_wait_obj(text));
+
+        if (g_get_sync_event() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_sync_event");
+        }
+
+        exit_status = xrdp_listen_main_loop(g_listen);
     }
 
-    g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid);
-    g_set_sigchld_event(g_create_wait_obj(text));
-
-    if (g_get_sigchld() == 0)
-    {
-        LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event");
-    }
-
-    g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid);
-    g_set_sync_event(g_create_wait_obj(text));
-
-    if (g_get_sync_event() == 0)
-    {
-        LOG(LOG_LEVEL_WARNING, "error creating g_sync_event");
-    }
-
-    g_listen->startup_params = &startup_params;
-    exit_status = xrdp_listen_main_loop(g_listen);
     xrdp_listen_delete(g_listen);
 
     tc_mutex_delete(g_get_sync_mutex());

xrdp/xrdp.h

@@ -194,13 +194,14 @@ xrdp_process_main_loop(struct xrdp_process *self);
 
 /* xrdp_listen.c */
 struct xrdp_listen *
-xrdp_listen_create(void);
+xrdp_listen_create(struct xrdp_startup_params *startup_params);
+int
+xrdp_listen_init(struct xrdp_listen *self);
 void
 xrdp_listen_delete(struct xrdp_listen *self);
 int
 xrdp_listen_main_loop(struct xrdp_listen *self);
-int
+
-xrdp_listen_test(struct xrdp_startup_params *startup_params);
 
 /* xrdp_region.c */
 struct xrdp_region *

xrdp/xrdp_listen.c

@@ -55,7 +55,7 @@ xrdp_listen_create_pro_done(struct xrdp_listen *self)
 
 /*****************************************************************************/
 struct xrdp_listen *
-xrdp_listen_create(void)
+xrdp_listen_create(struct xrdp_startup_params *startup_params)
 {
     struct xrdp_listen *self;
 
@@ -64,6 +64,7 @@ xrdp_listen_create(void)
     self->trans_list = list_create();
     self->process_list = list_create();
     self->fork_list = list_create();
+    self->startup_params = startup_params;
 
     if (g_process_sem == 0)
     {
@@ -153,107 +154,6 @@ xrdp_process_run(void *in_val)
     LOG_DEVEL(LOG_LEVEL_TRACE, "process done");
     return 0;
 }
-
-/*****************************************************************************/
-static int
-xrdp_listen_get_startup_params(struct xrdp_listen *self)
-{
-    int fd;
-    int index;
-    int port_override;
-    int fork_override;
-    const char *name;
-    const char *val;
-    struct list *names;
-    struct list *values;
-    struct xrdp_startup_params *startup_params;
-
-    startup_params = self->startup_params;
-    port_override = startup_params->port[0] != 0;
-    fork_override = startup_params->fork;
-    fd = g_file_open_ro(startup_params->xrdp_ini);
-    if (fd != -1)
-    {
-        names = list_create();
-        names->auto_free = 1;
-        values = list_create();
-        values->auto_free = 1;
-        if (file_read_section(fd, "globals", names, values) == 0)
-        {
-            for (index = 0; index < names->count; index++)
-            {
-                name = (const char *)list_get_item(names, index);
-                val = (const char *)list_get_item(values, index);
-                if (name == 0 || val == 0)
-                {
-                    continue;
-                }
-
-                if (g_strcasecmp(name, "port") == 0)
-                {
-                    if (port_override == 0)
-                    {
-                        g_strncpy(startup_params->port, val,
-                                  sizeof(startup_params->port) - 1);
-                    }
-                }
-
-                else if (g_strcasecmp(name, "fork") == 0)
-                {
-                    if (fork_override == 0)
-                    {
-                        startup_params->fork = g_text2bool(val);
-                    }
-                }
-
-                else if (g_strcasecmp(name, "tcp_nodelay") == 0)
-                {
-                    startup_params->tcp_nodelay = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_keepalive") == 0)
-                {
-                    startup_params->tcp_keepalive = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0)
-                {
-                    startup_params->tcp_send_buffer_bytes = g_atoi(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0)
-                {
-                    startup_params->tcp_recv_buffer_bytes = g_atoi(val);
-                }
-
-                else if (g_strcasecmp(name, "use_vsock") == 0)
-                {
-                    startup_params->use_vsock = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "runtime_user") == 0)
-                {
-                    g_snprintf(startup_params->runtime_user,
-                               sizeof(startup_params->runtime_user),
-                               "%s", val);
-                }
-
-                else if (g_strcasecmp(name, "runtime_group") == 0)
-                {
-                    g_snprintf(startup_params->runtime_group,
-                               sizeof(startup_params->runtime_group),
-                               "%s", val);
-                }
-            }
-        }
-
-        list_delete(names);
-        list_delete(values);
-        g_file_close(fd);
-    }
-    return 0;
-}
-
 /*****************************************************************************/
 static int
 xrdp_listen_stop_all_listen(struct xrdp_listen *self)
@@ -663,8 +563,10 @@ xrdp_listen_pp(struct xrdp_listen *self, int *index,
 }
 
 /*****************************************************************************/
-static int
+/* returns 0 if xrdp is listening correctly
-xrdp_listen_process_startup_params(struct xrdp_listen *self)
+   returns 1 if xrdp is not listening correctly */
+int
+xrdp_listen_init(struct xrdp_listen *self)
 {
     int mode; /* TRANS_MODE_TCP*, TRANS_MODE_UNIX, TRANS_MODE_VSOCK */
     int error;
@@ -898,18 +800,7 @@ xrdp_listen_main_loop(struct xrdp_listen *self)
     struct trans *ltrans;
 
     self->status = 1;
-    if (xrdp_listen_get_startup_params(self) != 0)
+
-    {
-        LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed");
-        self->status = -1;
-        return 1;
-    }
-    if (xrdp_listen_process_startup_params(self) != 0)
-    {
-        LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed");
-        self->status = -1;
-        return 1;
-    }
     term_obj = g_get_term(); /*Global termination event */
     sigchld_obj = g_get_sigchld();
     sync_obj = g_get_sync_event();
@@ -1049,27 +940,3 @@ xrdp_listen_main_loop(struct xrdp_listen *self)
     self->status = -1;
     return 0;
 }
-
-/*****************************************************************************/
-/* returns 0 if xrdp can listen
-   returns 1 if xrdp cannot listen */
-int
-xrdp_listen_test(struct xrdp_startup_params *startup_params)
-{
-    struct xrdp_listen *xrdp_listen;
-
-    xrdp_listen = xrdp_listen_create();
-    xrdp_listen->startup_params = startup_params;
-    if (xrdp_listen_get_startup_params(xrdp_listen) != 0)
-    {
-        xrdp_listen_delete(xrdp_listen);
-        return 1;
-    }
-    if (xrdp_listen_process_startup_params(xrdp_listen) != 0)
-    {
-        xrdp_listen_delete(xrdp_listen);
-        return 1;
-    }
-    xrdp_listen_delete(xrdp_listen);
-    return 0;
-}