From e2a582d5fd0d2671e1cf252170b6fdcf12a51840 Mon Sep 17 00:00:00 2001 From: Takase <20792268+takase1121@users.noreply.github.com> Date: Tue, 18 Apr 2023 02:56:04 +0800 Subject: [PATCH] Process API improvements (again) (#1370) * feat(process): add push_error * refactor(process): use push_error for better errors * style(process): consistent error messages * refactor(process): reimplement process.strerror() with push_error * refactor(process): implement close_fd only once * refactor(process): rename process_handle to process_handle_t * fix(process): prevent errors from a NULL error message * refactor(process): refactor push_error into 2 functions * fix(process): fix wrong error message * fix(process): check if push_error_string actually pushed something * refactor(process): make error messages descriptive * fix(process): check for empty table instead of aborting * refactor(process): make error messages descriptive on Windows * refactor(process): rename process_stream_handle to process_stream_t * refactor(process): fix wrong usage of process_handle_t * fix(process): fix wrong type name * refactor(process): incoporate kill_list_thread into process_kill_list_t * refactor(process): make kill_list per-state data --- src/api/process.c | 181 +++++++++++++++++++++++++++++++--------------- 1 file changed, 121 insertions(+), 60 deletions(-) diff --git a/src/api/process.c b/src/api/process.c index 2b0a5008..2e343c6c 100644 --- a/src/api/process.c +++ b/src/api/process.c @@ -24,13 +24,28 @@ #define READ_BUF_SIZE 2048 #define PROCESS_TERM_TRIES 3 #define PROCESS_TERM_DELAY 50 +#define PROCESS_KILL_LIST_NAME "__process_kill_list__" #if _WIN32 -typedef HANDLE process_stream_handle; -typedef HANDLE process_handle; + +typedef DWORD process_error_t; +typedef HANDLE process_stream_t; +typedef HANDLE process_handle_t; + +#define HANDLE_INVALID (INVALID_HANDLE_VALUE) +#define PROCESS_GET_HANDLE(P) ((P)->process_information.hProcess) + +static volatile long PipeSerialNumber; + #else -typedef int process_stream_handle; -typedef pid_t process_handle; + +typedef int process_error_t; +typedef int process_stream_t; +typedef pid_t process_handle_t; + +#define HANDLE_INVALID (0) +#define PROCESS_GET_HANDLE(P) ((P)->pid) + #endif typedef struct { @@ -43,13 +58,13 @@ typedef struct { bool reading[2]; char buffer[2][READ_BUF_SIZE]; #endif - process_stream_handle child_pipes[3][2]; + process_stream_t child_pipes[3][2]; } process_t; typedef struct process_kill_s { int tries; uint32_t start_time; - process_handle handle; + process_handle_t handle; struct process_kill_s *next; } process_kill_t; @@ -57,6 +72,7 @@ typedef struct { bool stop; SDL_mutex *mutex; SDL_cond *has_work, *work_done; + SDL_Thread *worker_thread; process_kill_t *head; process_kill_t *tail; } process_kill_list_t; @@ -83,21 +99,24 @@ typedef enum { REDIRECT_PARENT = -3, } filed_e; -static process_kill_list_t kill_list = { 0 }; -static SDL_Thread *kill_list_thread = NULL; - +static void close_fd(process_stream_t *handle) { + if (*handle) { #ifdef _WIN32 - static volatile long PipeSerialNumber; - static void close_fd(HANDLE* handle) { if (*handle) CloseHandle(*handle); *handle = INVALID_HANDLE_VALUE; } - #define PROCESS_GET_HANDLE(P) ((P)->process_information.hProcess) + CloseHandle(*handle); #else - static void close_fd(int* fd) { if (*fd) close(*fd); *fd = 0; } - #define PROCESS_GET_HANDLE(P) ((P)->pid) + close(*handle); #endif + *handle = HANDLE_INVALID; + } +} + + +static int kill_list_worker(void *ud); static void kill_list_free(process_kill_list_t *list) { process_kill_t *node, *temp; + SDL_WaitThread(list->worker_thread, NULL); SDL_DestroyMutex(list->mutex); SDL_DestroyCond(list->has_work); SDL_DestroyCond(list->work_done); @@ -107,10 +126,12 @@ static void kill_list_free(process_kill_list_t *list) { node = node->next; free(temp); } + memset(list, 0, sizeof(process_kill_list_t)); } static bool kill_list_init(process_kill_list_t *list) { + memset(list, 0, sizeof(process_kill_list_t)); list->mutex = SDL_CreateMutex(); list->has_work = SDL_CreateCond(); list->work_done = SDL_CreateCond(); @@ -120,6 +141,11 @@ static bool kill_list_init(process_kill_list_t *list) { kill_list_free(list); return false; } + list->worker_thread = SDL_CreateThread(kill_list_worker, "process_kill", list); + if (!list->worker_thread) { + kill_list_free(list); + return false; + } return true; } @@ -157,7 +183,7 @@ static void kill_list_wait_all(process_kill_list_t *list) { } -static void process_handle_close(process_handle *handle) { +static void process_handle_close(process_handle_t *handle) { #ifdef _WIN32 if (*handle) { CloseHandle(*handle); @@ -168,7 +194,7 @@ static void process_handle_close(process_handle *handle) { } -static bool process_handle_is_running(process_handle handle, int *status) { +static bool process_handle_is_running(process_handle_t handle, int *status) { #ifdef _WIN32 DWORD s; if (GetExitCodeProcess(handle, &s) && s != STILL_ACTIVE) { @@ -188,7 +214,7 @@ static bool process_handle_is_running(process_handle handle, int *status) { } -static bool process_handle_signal(process_handle handle, signal_e sig) { +static bool process_handle_signal(process_handle_t handle, signal_e sig) { #if _WIN32 switch(sig) { case SIGNAL_TERM: return GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, GetProcessId(handle)); @@ -252,6 +278,37 @@ static int kill_list_worker(void *ud) { } +static int push_error_string(lua_State *L, process_error_t err) { +#ifdef _WIN32 + char *msg = NULL; + FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM + | FORMAT_MESSAGE_ALLOCATE_BUFFER + | FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, + err, + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + (char *) &msg, + 0, + NULL); + if (!msg) + return 0; + + lua_pushstring(L, msg); + LocalFree(msg); +#else + lua_pushstring(L, strerror(err)); +#endif + return 1; +} + +static void push_error(lua_State *L, const char *extra, process_error_t err) { + const char *msg = "unknown error"; + extra = extra != NULL ? extra : "error"; + if (push_error_string(L, err)) + msg = lua_tostring(L, -1); + lua_pushfstring(L, "%s: %s (%d)", extra, msg, err); +} + static bool poll_process(process_t* proc, int timeout) { uint32_t ticks; @@ -296,6 +353,10 @@ static int process_start(lua_State* L) { lua_pushinteger(L, (int)lua_objlen(L, 1)); #endif cmd_len = luaL_checknumber(L, -1); lua_pop(L, 1); + if (!cmd_len) + // we have not allocated anything here yet, so we can skip cleanup code + // don't do this anywhere else! + return luaL_argerror(L, 1,"table cannot be empty"); for (size_t i = 1; i <= cmd_len; ++i) { lua_pushinteger(L, i); lua_rawget(L, 1); @@ -306,9 +367,6 @@ static int process_start(lua_State* L) { cmd[0] = luaL_checkstring(L, 1); cmd_len = 1; } - // this should never trip - // but if it does we are in deep trouble - assert(cmd[0]); if (arg_len > 1) { lua_getfield(L, 2, "env"); @@ -334,7 +392,7 @@ static int process_start(lua_State* L) { lua_getfield(L, 2, "stderr"); new_fds[STDERR_FD] = luaL_optnumber(L, -1, STDERR_FD); for (int stream = STDIN_FD; stream <= STDERR_FD; ++stream) { if (new_fds[stream] > STDERR_FD || new_fds[stream] < REDIRECT_PARENT) { - lua_pushfstring(L, "redirect to handles, FILE* and paths are not supported"); + lua_pushfstring(L, "error: redirect to handles, FILE* and paths are not supported"); retval = -1; goto cleanup; } @@ -368,20 +426,22 @@ static int process_start(lua_State* L) { self->child_pipes[i][0] = CreateNamedPipeA(pipeNameBuffer, PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED, PIPE_TYPE_BYTE | PIPE_WAIT, 1, READ_BUF_SIZE, READ_BUF_SIZE, 0, NULL); if (self->child_pipes[i][0] == INVALID_HANDLE_VALUE) { - lua_pushfstring(L, "Error creating read pipe: %d.", GetLastError()); + push_error(L, "cannot create pipe", GetLastError()); retval = -1; goto cleanup; } self->child_pipes[i][1] = CreateFileA(pipeNameBuffer, GENERIC_WRITE, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); if (self->child_pipes[i][1] == INVALID_HANDLE_VALUE) { + // prevent CloseHandle from messing up error codes + DWORD err = GetLastError(); CloseHandle(self->child_pipes[i][0]); - lua_pushfstring(L, "Error creating write pipe: %d.", GetLastError()); + push_error(L, "cannot open pipe", err); retval = -1; goto cleanup; } if (!SetHandleInformation(self->child_pipes[i][i == STDIN_FD ? 1 : 0], HANDLE_FLAG_INHERIT, 0) || !SetHandleInformation(self->child_pipes[i][i == STDIN_FD ? 0 : 1], HANDLE_FLAG_INHERIT, 1)) { - lua_pushfstring(L, "Error inheriting pipes: %d.", GetLastError()); + push_error(L, "cannot set pipe permission", GetLastError()); retval = -1; goto cleanup; } @@ -445,7 +505,7 @@ static int process_start(lua_State* L) { if (env_len > 0) MultiByteToWideChar(CP_UTF8, MB_PRECOMPOSED, environmentBlock, offset, (LPWSTR)wideEnvironmentBlock, sizeof(wideEnvironmentBlock)); if (!CreateProcess(NULL, commandLine, NULL, NULL, true, (detach ? DETACHED_PROCESS : CREATE_NO_WINDOW) | CREATE_UNICODE_ENVIRONMENT, env_len > 0 ? wideEnvironmentBlock : NULL, cwd, &siStartInfo, &self->process_information)) { - lua_pushfstring(L, "Error creating a process: %d.", GetLastError()); + push_error(L, NULL, GetLastError()); retval = -1; goto cleanup; } @@ -457,7 +517,7 @@ static int process_start(lua_State* L) { int control_pipe[2] = { 0 }; for (int i = 0; i < 3; ++i) { // Make only the parents fd's non-blocking. Children should block. if (pipe(self->child_pipes[i]) || fcntl(self->child_pipes[i][i == STDIN_FD ? 1 : 0], F_SETFL, O_NONBLOCK) == -1) { - lua_pushfstring(L, "Error creating pipes: %s", strerror(errno)); + push_error(L, "cannot create pipe", errno); retval = -1; goto cleanup; } @@ -476,7 +536,7 @@ static int process_start(lua_State* L) { self->pid = (long)fork(); if (self->pid < 0) { - lua_pushfstring(L, "Error running fork: %s.", strerror(errno)); + push_error(L, "cannot create child process", errno); retval = -1; goto cleanup; } else if (!self->pid) { @@ -533,7 +593,7 @@ static int process_start(lua_State* L) { free((char*)env_values[i]); } for (int stream = 0; stream < 3; ++stream) { - process_stream_handle* pipe = &self->child_pipes[stream][stream == STDIN_FD ? 0 : 1]; + process_stream_t* pipe = &self->child_pipes[stream][stream == STDIN_FD ? 0 : 1]; if (*pipe) { close_fd(pipe); } @@ -549,7 +609,7 @@ static int g_read(lua_State* L, int stream, unsigned long read_size) { process_t* self = (process_t*) luaL_checkudata(L, 1, API_TYPE_PROCESS); long length = 0; if (stream != STDOUT_FD && stream != STDERR_FD) - return luaL_error(L, "redirect to handles, FILE* and paths are not supported"); + return luaL_error(L, "error: redirect to handles, FILE* and paths are not supported"); #if _WIN32 int writable_stream_idx = stream - 1; if (self->reading[writable_stream_idx] || !ReadFile(self->child_pipes[stream][0], self->buffer[writable_stream_idx], READ_BUF_SIZE, NULL, &self->overlapped[writable_stream_idx])) { @@ -597,9 +657,9 @@ static int f_write(lua_State* L) { #if _WIN32 DWORD dwWritten; if (!WriteFile(self->child_pipes[STDIN_FD][1], data, data_size, &dwWritten, NULL)) { - int lastError = GetLastError(); + push_error(L, NULL, GetLastError()); signal_process(self, SIGNAL_TERM); - return luaL_error(L, "error writing to process: %d", lastError); + return lua_error(L); } length = dwWritten; #else @@ -607,9 +667,9 @@ static int f_write(lua_State* L) { if (length < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) length = 0; else if (length < 0) { - const char* lastError = strerror(errno); + push_error(L, "cannot write to child process", errno); signal_process(self, SIGNAL_TERM); - return luaL_error(L, "error writing to process: %s", lastError); + return lua_error(L); } #endif lua_pushinteger(L, length); @@ -626,15 +686,7 @@ static int f_close_stream(lua_State* L) { // Generic stuff below here. static int process_strerror(lua_State* L) { - #if _WIN32 - return 1; - #endif - int error_code = luaL_checknumber(L, 1); - if (error_code < 0) - lua_pushstring(L, strerror(error_code)); - else - lua_pushnil(L); - return 1; + return push_error_string(L, luaL_checknumber(L, 1)); } static int f_tostring(lua_State* L) { @@ -688,30 +740,32 @@ static int f_kill(lua_State* L) { return self_signal(L, SIGNAL_KILL); } static int f_interrupt(lua_State* L) { return self_signal(L, SIGNAL_INTERRUPT); } static int f_gc(lua_State* L) { + process_kill_list_t *list = NULL; + process_kill_t *p = NULL; process_t* self = (process_t*) luaL_checkudata(L, 1, API_TYPE_PROCESS); - if (poll_process(self, 0) && !self->detached) { - // attempt to kill the process if not detached - process_kill_t *p; + // get the kill_list for the lua_State + if (lua_getfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME) == LUA_TUSERDATA) + list = (process_kill_list_t *) lua_touserdata(L, -1); + if (poll_process(self, 0) && !self->detached) { + // attempt to kill the process if still running and not detached signal_process(self, SIGNAL_TERM); - p = malloc(sizeof(process_kill_t)); - if (!p || !kill_list_thread) { - // if we can't allocate, we'll use the old method - poll_process(self, PROCESS_TERM_DELAY); - if (self->running) { + if (!list || !list->worker_thread || !(p = malloc(sizeof(process_kill_t)))) { + // use synchronous waiting + if (poll_process(self, PROCESS_TERM_DELAY)) { signal_process(self, SIGNAL_KILL); poll_process(self, PROCESS_TERM_DELAY); } } else { - // send the handle to a queue for asynchronous waiting + // put the handle into a queue for asynchronous waiting p->handle = PROCESS_GET_HANDLE(self); p->start_time = SDL_GetTicks(); p->tries = 1; - SDL_LockMutex(kill_list.mutex); - kill_list_push(&kill_list, p); - SDL_CondSignal(kill_list.has_work); - SDL_UnlockMutex(kill_list.mutex); + SDL_LockMutex(list->mutex); + kill_list_push(list, p); + SDL_CondSignal(list->has_work); + SDL_UnlockMutex(list->mutex); } } close_fd(&self->child_pipes[STDIN_FD ][1]); @@ -727,9 +781,13 @@ static int f_running(lua_State* L) { } static int process_gc(lua_State *L) { - kill_list_wait_all(&kill_list); - SDL_WaitThread(kill_list_thread, NULL); - kill_list_free(&kill_list); + process_kill_list_t *list = NULL; + // get the kill_list for the lua_State + if (lua_getfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME) == LUA_TUSERDATA) { + list = (process_kill_list_t *) lua_touserdata(L, -1); + kill_list_wait_all(list); + kill_list_free(list); + } return 0; } @@ -758,8 +816,11 @@ static const struct luaL_Reg lib[] = { }; int luaopen_process(lua_State *L) { - if (kill_list_init(&kill_list)) - kill_list_thread = SDL_CreateThread(kill_list_worker, "process_kill", &kill_list); + process_kill_list_t *list = lua_newuserdata(L, sizeof(process_kill_list_t)); + if (kill_list_init(list)) + lua_setfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME); + else + lua_pop(L, 1); // discard the list // create the process metatable luaL_newmetatable(L, API_TYPE_PROCESS);