From bee806d3afc59fabbeb12be8df6a6e913ebdaf4a Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 6 Dec 2024 12:12:24 +0000 Subject: [PATCH] Add sesexec discover module The module is a dummy to be filled in later Other structural changes to sesexec:- 1) A failure of sesman needs to be detected and handled without causing sesexec to exit 2) If sesexec exits, the session can never be rediscovered. sesexec must be robust enough to stay up for the lifetime of the session so that the discovery function always works. 3) There is a mechanism for sesexec to terminate the session, but it doesn't work, as SIGCHLD is not processed while we are waiting for the session to finish. This needs fixing. --- sesman/sesexec/Makefile.am | 2 + sesman/sesexec/eicp_server.c | 89 ++++++++++++++------- sesman/sesexec/sesexec.c | 125 +++++++++++++++++++++++++----- sesman/sesexec/sesexec_discover.c | 63 +++++++++++++++ sesman/sesexec/sesexec_discover.h | 77 ++++++++++++++++++ 5 files changed, 309 insertions(+), 47 deletions(-) create mode 100644 sesman/sesexec/sesexec_discover.c create mode 100644 sesman/sesexec/sesexec_discover.h diff --git a/sesman/sesexec/Makefile.am b/sesman/sesexec/Makefile.am index 1eae19cb..ac8a2c42 100644 --- a/sesman/sesexec/Makefile.am +++ b/sesman/sesexec/Makefile.am @@ -29,6 +29,8 @@ xrdp_sesexec_SOURCES = \ env.h \ login_info.c \ login_info.h \ + sesexec_discover.c \ + sesexec_discover.h \ sessionrecord.c \ sessionrecord.h \ xauth.c \ diff --git a/sesman/sesexec/eicp_server.c b/sesman/sesexec/eicp_server.c index 80255fdf..210d21b7 100644 --- a/sesman/sesexec/eicp_server.c +++ b/sesman/sesexec/eicp_server.c @@ -37,6 +37,7 @@ #include "ercp.h" #include "scp.h" #include "sesexec.h" +#include "sesexec_discover.h" #include "session.h" /******************************************************************************/ @@ -98,12 +99,13 @@ handle_create_session_request(struct trans *self) { int scp_fd; struct session_parameters sp = {0}; - int rv; + int status; - rv = eicp_get_create_session_request(self, &scp_fd, &sp.display, - &sp.type, &sp.width, &sp.height, - &sp.bpp, &sp.shell, &sp.directory); - if (rv == 0) + status = eicp_get_create_session_request( + self, &scp_fd, &sp.display, + &sp.type, &sp.width, &sp.height, + &sp.bpp, &sp.shell, &sp.directory); + if (status == 0) { // Need to talk to the SCP client struct trans *scp_trans; @@ -113,7 +115,7 @@ handle_create_session_request(struct trans *self) { LOG(LOG_LEVEL_ERROR, "Can't create SCP trans"); g_file_close(scp_fd); - rv = 1; + status = 1; } else { @@ -134,39 +136,68 @@ handle_create_session_request(struct trans *self) scp_status = session_start(g_login_info, &sp, &g_session_data); } - // Return the status to the SCP client - rv = scp_send_create_session_response(scp_trans, scp_status, - sp.display, &sp.guid); - trans_delete(scp_trans); - - // Further comms from sesexec is sent over the ERCP protocol + // Further comms to sesman is sent over the ERCP protocol ercp_trans_from_eicp_trans(self, sesexec_ercp_data_in, (void *)self); - if (scp_status == E_SCP_SCREATE_OK) + if (scp_status != E_SCP_SCREATE_OK) { - rv = ercp_send_session_announce_event( - self, - sp.display, - g_login_info->uid, - sp.type, - sp.width, - sp.height, - sp.bpp, - &sp.guid, - g_login_info->ip_addr, - session_get_start_time(g_session_data)); + // Tell sesman the session hasn't started + (void)ercp_send_session_finished_event(self); } - else + else if ((status = ercp_send_session_announce_event( + self, + sp.display, + g_login_info->uid, + sp.type, + sp.width, + sp.height, + sp.bpp, + &sp.guid, + g_login_info->ip_addr, + session_get_start_time(g_session_data))) != 0) { - rv = ercp_send_session_finished_event(self); - sesexec_terminate_main_loop(1); + // We failed to tell sesman about the new session. This + // probably means sesman has exited in the time between + // asking us to start a session, and our reply. This + // could be many seconds, and a new sesman may well + // have started. + // If we enable the restart functionality at + // this point, we have a race condition that could + // result in a session which sesman doesn't know + // about. The simplest thing to do in this rare situation + // is to abort the session - the user can create a + // new one + LOG(LOG_LEVEL_ERROR, + "sesman appears to have failed - stopping session"); + scp_status = E_SCP_SCREATE_GENERAL_ERROR; + } + else if ((status = sesexec_discover_enable()) != 0) + { + // Equally regrettable - we can't make the session + // discoverable, so we'll stop it. We can tell sesman + // about this one however. + LOG(LOG_LEVEL_ERROR, + "unable to make session discoverable" + " - stopping session"); + (void)ercp_send_session_finished_event(self); + scp_status = E_SCP_SCREATE_GENERAL_ERROR; } - } + // Return the status to the SCP client. + (void)scp_send_create_session_response(scp_trans, scp_status, + sp.display, &sp.guid); + trans_delete(scp_trans); + } } - return rv; + + if (status != 0) + { + // Kill sesexec, and any active session + sesexec_terminate_main_loop(status); + } + return 0; } /******************************************************************************/ diff --git a/sesman/sesexec/sesexec.c b/sesman/sesexec/sesexec.c index 5fe52630..752a7709 100644 --- a/sesman/sesexec/sesexec.c +++ b/sesman/sesexec/sesexec.c @@ -38,6 +38,7 @@ #include "ercp_server.h" #include "login_info.h" #include "sesexec.h" +#include "sesexec_discover.h" #include "sesman_config.h" #include "log.h" #include "os_calls.h" @@ -209,8 +210,12 @@ sesexec_is_term(void) void sesexec_terminate_main_loop(int status) { - g_terminate_loop = 1; - g_terminate_status = status; + // Only take the first request to terminate the loop + if (!g_terminate_loop) + { + g_terminate_loop = 1; + g_terminate_status = status; + } } /******************************************************************************/ @@ -227,6 +232,45 @@ process_sigchld_event(void) } } +/******************************************************************************/ +static void +sesexec_terminate_session_and_wait(void) +{ + session_send_term(g_session_data); + do + { + g_sleep(1000); + // Process SIGCHLD events while waiting for the session + // to exit. + if (g_is_wait_obj_set(g_sigchld_event)) + { + g_reset_wait_obj(g_sigchld_event); + process_sigchld_event(); + } + } + while (session_active(g_session_data)); +} + +/******************************************************************************/ +static void +sesexec_main_loop_cleanup(void) +{ + login_info_free(g_login_info); + + /* This session is no longer discoverable */ + sesexec_discover_disable(); + + /* Don't allow sesexec to terminate with an active + session, as we can't connect to such a session */ + if (session_active(g_session_data)) + { + LOG(LOG_LEVEL_INFO, + "Stopping session on xrdp-sesexec exit"); + sesexec_terminate_session_and_wait(); + } + session_data_free(g_session_data); +} + /******************************************************************************/ /** * @@ -238,7 +282,8 @@ sesexec_main_loop(void) { int error = 0; int robjs_count; - intptr_t robjs[32]; +#define MAX_ROBJS 32 + intptr_t robjs[MAX_ROBJS]; g_terminate_loop = 0; g_terminate_status = 0; @@ -250,11 +295,25 @@ sesexec_main_loop(void) robjs[robjs_count++] = g_term_event; robjs[robjs_count++] = g_sigchld_event; - error = trans_get_wait_objs(g_ecp_trans, robjs, &robjs_count); + // ECP transport may be null if sesman has gone away + if (g_ecp_trans != NULL) + { + error = trans_get_wait_objs(g_ecp_trans, robjs, &robjs_count); + if (error != 0) + { + LOG(LOG_LEVEL_ERROR, "sesexec_main_loop: " + "trans_get_wait_objs(ECP) failed"); + sesexec_terminate_main_loop(error); + continue; + } + } + + // Add any objects from the discover module + error = sesexec_discover_get_wait_objs(robjs, &robjs_count, MAX_ROBJS); if (error != 0) { LOG(LOG_LEVEL_ERROR, "sesexec_main_loop: " - "trans_get_wait_objs(ECP) failed"); + "sesexec_discover_get_wait_objs() failed"); sesexec_terminate_main_loop(error); continue; } @@ -273,21 +332,17 @@ sesexec_main_loop(void) g_reset_wait_obj(g_term_event); if (session_active(g_session_data)) { - // Ask the active session to terminate LOG(LOG_LEVEL_INFO, "sesexec_main_loop: " - "sesexec asked to terminate. " - "Terminating active session"); - session_send_term(g_session_data); + "sesexec asked to terminate with active session."); } else { - // Terminate immediately LOG(LOG_LEVEL_INFO, "sesexec_main_loop: " "sesexec asked to terminate. " "No session is active"); - sesexec_terminate_main_loop(0); - continue; } + sesexec_terminate_main_loop(0); + continue; } if (g_is_wait_obj_set(g_sigchld_event)) /* SIGCHLD */ @@ -302,7 +357,10 @@ sesexec_main_loop(void) { // We've finished the session. Tell sesman and // finish up. - (void)ercp_send_session_finished_event(g_ecp_trans); + if (g_ecp_trans != NULL) + { + (void)ercp_send_session_finished_event(g_ecp_trans); + } session_data_free(g_session_data); g_session_data = NULL; @@ -311,19 +369,51 @@ sesexec_main_loop(void) } } - error = trans_check_wait_objs(g_ecp_trans); + if (g_ecp_trans != NULL) + { + error = trans_check_wait_objs(g_ecp_trans); + if (error != 0) + { + if (g_ecp_trans->status != TRANS_STATUS_UP && + session_active(g_session_data)) + { + // sesman has gone away. We have an active session + // to keep track of, so sesman can pick it up when it + // restarts + LOG(LOG_LEVEL_INFO, "sesexec_main_loop: " + "sesman has exited"); + trans_delete(g_ecp_trans); + g_ecp_trans = NULL; + } + else + { + // A callback has failed, or sesman has gone away and + // we have no active session + LOG(LOG_LEVEL_ERROR, "sesexec_main_loop: " + "trans_check_wait_objs failed for ECP transport"); + sesexec_terminate_main_loop(error); + } + continue; + } + } + + error = sesexec_discover_check_wait_objs(); if (error != 0) { LOG(LOG_LEVEL_ERROR, "sesexec_main_loop: " - "trans_check_wait_objs failed for ECP transport"); + "sesexec_discover_check_wait_objs failed"); sesexec_terminate_main_loop(error); continue; } + } - login_info_free(g_login_info); + /* close sesman communications immediately */ + trans_delete(g_ecp_trans); + g_ecp_trans = NULL; return g_terminate_status; +#undef MAX_ROBJS } /******************************************************************************/ @@ -483,9 +573,8 @@ main(int argc, char **argv) /* start program main loop */ LOG(LOG_LEVEL_INFO, "starting xrdp-sesexec with pid %d", g_pid); error = sesexec_main_loop(); - trans_delete(g_ecp_trans); + sesexec_main_loop_cleanup(); } - g_delete_wait_obj(g_term_event); } config_free(g_cfg); diff --git a/sesman/sesexec/sesexec_discover.c b/sesman/sesexec/sesexec_discover.c new file mode 100644 index 00000000..d4a2ea7f --- /dev/null +++ b/sesman/sesexec/sesexec_discover.c @@ -0,0 +1,63 @@ +/** + * xrdp: A Remote Desktop Protocol server. + * + * Copyright (C) Jay Sorg 2004-2024 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * + * @file sesexec_discover.c + * @brief Declare functionality associated with sesman restart support + * @author Matt Burt + * + */ + +#if defined(HAVE_CONFIG_H) +#include +#endif + +#include "sesexec_discover.h" + +/******************************************************************************/ +int +sesexec_discover_enable(void) +{ + return 0; +} + +/******************************************************************************/ + +int +sesexec_discover_disable(void) +{ + return 0; +} + +/******************************************************************************/ + +int +sesexec_discover_get_wait_objs(intptr_t robjs[], int *robjs_count, + int max_count) +{ + return 0; +} + +/******************************************************************************/ + +int +sesexec_discover_check_wait_objs(void) +{ + return 0; +} diff --git a/sesman/sesexec/sesexec_discover.h b/sesman/sesexec/sesexec_discover.h new file mode 100644 index 00000000..e29fea40 --- /dev/null +++ b/sesman/sesexec/sesexec_discover.h @@ -0,0 +1,77 @@ +/** + * xrdp: A Remote Desktop Protocol server. + * + * Copyright (C) Jay Sorg 2004-2024 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * + * @file sesexec_discover.h + * @brief Declare functionality associated with sesman restart support + * for sesexec + * + * @author Matt Burt + * + */ + +#ifndef SESEXEC_DISCOVER_H +#define SESEXEC_DISCOVER_H + +#include + +/** + * Start the listening object used when sesman restarts + * + * @return != 0 for error + */ +int +sesexec_discover_enable(void); + +/** + * Stop the listening object used when sesman restarts, and deallocate all + * module resources. + * + * @return != 0 for error + */ +int +sesexec_discover_disable(void); + +/** + * Add any file descriptors in use by the module to an array + * + * @param robjs Array to add fds to + * @param[in,out] robjs_count Index where elements are to be added + * @param max_count Max value of robjs_count + * @return != 0 for error + * + * This function can be called before sesexec_discover_enable(), + * in which case it does nothing. + */ +int +sesexec_discover_get_wait_objs(intptr_t robjs[], int *robjs_count, + int max_count); + + +/** + * Check any file descriptors in use by the module for actionable events + * @return != 0 for error + * + * This function can be called before sesexec_discover_enable(), + * in which case it does nothing. + */ +int +sesexec_discover_check_wait_objs(void); + +#endif // SESEXEC_DISCOVER_H