diff --git a/.kiro/steering/tech.md b/.kiro/steering/tech.md index 64ba8f69..8e792f25 100644 --- a/.kiro/steering/tech.md +++ b/.kiro/steering/tech.md @@ -91,7 +91,7 @@ Early return on every error. Log with zap fields before returning. - **Threaded processes**: the controller, the scheduler, the `xcecd` daemon (`xcecd`, `xcecd_virtual`: thread pool, network, resource-sync and connector threads), and any process that starts libxce's delaymgr/netio/vtaskmgr threads (`xce_ctx_start()`). - **How to start a child instead**: `posix_spawn()`/`posix_spawnp()` (glibc runs it with `CLONE_VM|CLONE_VFORK`, no copied user code), or a systemd unit over D-Bus as `xcecd` does for `xcecd_worker`. Setup that `posix_spawn()` cannot do (setuid, rlimits, cgroup join) belongs in a single-threaded process such as `xcecd_worker`, never in the threaded daemon. - **Allowed**: `exec*()` without a fork (process replacement, e.g. the controller's HA self-restart); and a fork-family call in a process proven single-threaded, marked on the call line or the line above with `/* xce-fork-ok: */`. `xcecd_worker` (the per-job process) is single-threaded: its call graph from `main()`, callbacks included, reaches no `pthread_create()` and no libuv thread-pool call (`uv_queue_work`, `uv_fs_*`, `uv_getaddrinfo`); re-prove it before adding a thread there. -- **Scope**: C code under `src/controller`, `src/scheduler`, `src/lib`, `src/worker`. Excluded (decision 2026-10-10): test code (`testsuite/`, `test*` files; forking to isolate a test case is fine), the harness (`src/harness`, `xce_test`), the autoscaler (not finalized; its `plug_mgr.c` fork is tracked as a bug and must be fixed before it ships), and the Go frontend (`os/exec` is safe). +- **Scope**: C code under `src/controller`, `src/scheduler`, `src/lib`, `src/worker`. Excluded (decision 2026-10-10): test code (`testsuite/`, `test*` files; forking to isolate a test case is fine), the harness (`src/harness`, `xce_test`), the autoscaler (not finalized; its provider calls use `posix_spawn()` since bug-1969, and its suite counts `fork()` calls through `-Wl,--wrap=fork`), and the Go frontend (`os/exec` is safe). - **Enforcement**: `scripts/check-no-fork.py` runs as the meson test `no-fork rule` (suites lib, controller, scheduler, worker, so `/xce-build` runs it) and in the pre-commit hook on staged C files. ### Auth and RBAC diff --git a/scripts/check-no-fork.py b/scripts/check-no-fork.py index 09bfcf7e..2a69c433 100644 --- a/scripts/check-no-fork.py +++ b/scripts/check-no-fork.py @@ -9,7 +9,8 @@ started with posix_spawn(), or by systemd over D-Bus. Scanned: C sources of the components the rule covers (COVERED below), minus test code. Excluded by decision (2026-10-10): tests, the harness (xce_test), -the autoscaler (not finalized; tracked as a bug) and the Go frontend. +the autoscaler (not finalized; provider calls use posix_spawn since bug-1969) +and the Go frontend. A call is allowed only in a process proven single-threaded, marked on the call line or the line above with: /* xce-fork-ok: */ diff --git a/src/autoscaler/meson.build b/src/autoscaler/meson.build index d47d7349..d7b2e8c1 100644 --- a/src/autoscaler/meson.build +++ b/src/autoscaler/meson.build @@ -24,3 +24,19 @@ libscaler = static_library('scaler', install: true) subdir('providers') + +# Scaler unit tests: plug_mgr drives the fake provider scripts in +# scaler/testsuite/scripts (scriptPath "./testsuite"), so they run from +# scaler/. --wrap=fork: the no-fork rule (tech.md "Process Creation", +# bug-1969) -- the tests count fork() calls made while invoking a provider. +scaler_test = executable('scaler_test', + ['scaler/testsuite/test.c', + 'scaler/testsuite/test_plug_mgr.c', + 'scaler/testsuite/test_scale_misc.c'], + include_directories: [incdir], + dependencies: [libscaler_deps, yaml_deps, unity_dep], + link_with: libscaler, + link_args: ['-Wl,--wrap=fork']) +test('autoscaler test', scaler_test, + workdir: meson.current_source_dir() / 'scaler', + suite: 'autoscaler') diff --git a/src/autoscaler/scaler/plug_mgr.c b/src/autoscaler/scaler/plug_mgr.c index 98e72297..c47c6bde 100644 --- a/src/autoscaler/scaler/plug_mgr.c +++ b/src/autoscaler/scaler/plug_mgr.c @@ -6,13 +6,19 @@ #include "scaler_obj.h" #include "utils.h" #include +#include #include +#include +#include #include #include #include +#include #include #include +extern char** environ; + static cJSON* invoke(scaler_provider_t* prov, INTERFACE_ACTION ACTION, cJSON* in_json); @@ -379,18 +385,124 @@ int get_return_request(char* prov_name, instance_id_list_t* inst_ids) return 0; } -// PRO_CONF_DIR -// PRO_DATA_DIR -// PRO_LOG_DIR -// PROVIDER_NAME default to "baidu" if not set -// TODO zcg need to set PRO_LOG_DIR -static int export_env(scaler_provider_t* prov) +static void free_env(char** envp) { - setenv("PRO_CONF_DIR", prov->confPath, 1); - setenv("PRO_DATA_DIR", "/tmp", 1); - setenv("PRO_LOG_DIR", "/tmp", 1); - setenv("PROVIDER_NAME", prov->name, 1); - return 0; + if (!envp) { + return; + } + for (char** e = envp; *e; e++) { + free(*e); + } + free(envp); +} + +/* + * provider_env - the environment of a provider script: the scaler's own + * environment with PRO_CONF_DIR, PRO_DATA_DIR, PRO_LOG_DIR and + * PROVIDER_NAME set for this provider. Built for posix_spawn() so that + * neither the scaler (multi-threaded: setenv() is not thread-safe) nor a + * forked child (bug-1969) has to setenv(). Returns NULL on allocation + * failure; free with free_env(). + */ +// TODO zcg need to set PRO_LOG_DIR +static char** provider_env(scaler_provider_t* prov) +{ + const char* names[] = { "PRO_CONF_DIR", "PRO_DATA_DIR", "PRO_LOG_DIR", + "PROVIDER_NAME" }; + const char* values[] = { prov->confPath, "/tmp", "/tmp", prov->name }; + size_t nset = sizeof(names) / sizeof(names[0]); + size_t nenv = 0; + size_t k = 0; + char** envp; + + while (environ && environ[nenv]) { + nenv++; + } + envp = calloc(nenv + nset + 1, sizeof(char*)); + if (!envp) { + return NULL; + } + for (size_t i = 0; i < nenv; i++) { + bool replaced = false; + for (size_t j = 0; j < nset && !replaced; j++) { + size_t len = strlen(names[j]); + replaced = strncmp(environ[i], names[j], len) == 0 + && environ[i][len] == '='; + } + if (!replaced && !(envp[k++] = strdup(environ[i]))) { + free_env(envp); + return NULL; + } + } + for (size_t j = 0; j < nset; j++) { + if (asprintf(&envp[k++], "%s=%s", names[j], values[j] ? values[j] : "") + < 0) { + envp[k - 1] = NULL; + free_env(envp); + return NULL; + } + } + return envp; +} + +/* + * run_script - run `sh -f ` with stdout and stderr in + * out_file and wait for it. posix_spawn(), not fork(): the scaler is + * multi-threaded, and a forked child that logged, allocated or setenv()'d + * before exec could deadlock on a lock another thread held (bug-1969, + * tech.md "Process Creation"). The child starts with an empty signal mask + * and default signal dispositions. Returns the wait status, or -errno. + */ +static int run_script(scaler_provider_t* prov, const char* cmd, + const char* in_file, const char* out_file) +{ + posix_spawn_file_actions_t fa; + posix_spawnattr_t attr; + sigset_t none, all; + char* argv[] = { "sh", (char*)cmd, "-f", (char*)in_file, NULL }; + char** envp = provider_env(prov); + pid_t pid; + int status = 0; + int rc; + + if (!envp) { + return -ENOMEM; + } + sigemptyset(&none); + sigfillset(&all); + posix_spawn_file_actions_init(&fa); + posix_spawnattr_init(&attr); + rc = posix_spawn_file_actions_addopen(&fa, STDOUT_FILENO, out_file, + O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (rc == 0) { + rc = posix_spawn_file_actions_adddup2(&fa, STDOUT_FILENO, + STDERR_FILENO); + } + if (rc == 0) { + rc = posix_spawnattr_setflags( + &attr, POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF); + } + if (rc == 0) { + rc = posix_spawnattr_setsigmask(&attr, &none); + } + if (rc == 0) { + rc = posix_spawnattr_setsigdefault(&attr, &all); + } + if (rc == 0) { + rc = posix_spawn(&pid, "/bin/sh", &fa, &attr, argv, envp); + } + posix_spawnattr_destroy(&attr); + posix_spawn_file_actions_destroy(&fa); + free_env(envp); + if (rc != 0) { + return -rc; + } + while (waitpid(pid, &status, 0) < 0) { + if (errno != EINTR) { + return -errno; + } + } + return status; } static int write_input_json(const char* filename, cJSON* json) @@ -422,9 +534,9 @@ static cJSON* invoke(scaler_provider_t* prov, INTERFACE_ACTION ACTION, { char in_file[64] = "/tmp/sc_in_XXXXXX"; char out_file[64] = "/tmp/sc_out_XXXXXX"; + char cmd[XCE_MAXPATH_LEN]; cJSON* out_json = NULL; - int ret; - pid_t pid; + int status; if (!prov || !in_json) { ERROR("Invalid input JSON"); @@ -437,50 +549,17 @@ static cJSON* invoke(scaler_provider_t* prov, INTERFACE_ACTION ACTION, } write_input_json(in_file, in_json); - // Fork and exec the cmd to run the provider script - pid = fork(); - if (pid == -1) { - ERROR("fork() failed"); + // Run the provider script and wait for it + my_snprintf(cmd, sizeof(cmd), "%s/scripts/%s", prov->scriptPath, + prov->scripts[ACTION]); + status = run_script(prov, cmd, in_file, out_file); + if (status < 0) { + ERROR("Failed to run %s: %s", cmd, strerror(-status)); goto _OUT_; - } else if (pid == 0) { - char cmd[XCE_MAXPATH_LEN]; - int fd; - // Open output file (overwrite mode) - fd = open(out_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); - if (fd == -1) { - ERROR("Create %s failed", out_file); - exit(EXIT_FAILURE); - } - // Build command to call the script - my_snprintf(cmd, sizeof(cmd), "%s/scripts/%s", prov->scriptPath, - prov->scripts[ACTION]); - - // Redirect stdout and stderr to file - dup2(fd, STDOUT_FILENO); - dup2(fd, STDERR_FILENO); - close(fd); - - // export environment variables - export_env(prov); - // Execute script - ret = execl("/bin/sh", "sh", cmd, "-f", in_file, (char*)NULL); - - if (ret == -1) { - ERROR("execl() %s failed", cmd); - exit(EXIT_FAILURE); // Exit child process on failure - } - - // If exec succeeds, it won't return here - exit(EXIT_SUCCESS); + } else if (WIFEXITED(status)) { + TRACE("Script exited with status: %d", WEXITSTATUS(status)); } else { - int status; - waitpid(pid, &status, 0); - - if (WIFEXITED(status)) { - TRACE("Script exited with status: %d", WEXITSTATUS(status)); - } else { - INFO("Script terminated abnormally"); - } + INFO("Script terminated abnormally"); } read_json_file(out_file, &out_json); diff --git a/src/autoscaler/scaler/testsuite/envscripts/scripts/getAvailableTemplates.sh b/src/autoscaler/scaler/testsuite/envscripts/scripts/getAvailableTemplates.sh new file mode 100644 index 00000000..52daff4d --- /dev/null +++ b/src/autoscaler/scaler/testsuite/envscripts/scripts/getAvailableTemplates.sh @@ -0,0 +1,15 @@ +#!/bin/sh +# bug-1969: reports the provider environment the scaler gave this script. +cat < #include -#include "unity.h" +#include extern void test_plug_mgr(void); extern void test_scale_misc(void); diff --git a/src/autoscaler/scaler/testsuite/test_plug_mgr.c b/src/autoscaler/scaler/testsuite/test_plug_mgr.c index f5f41231..fd1beb5b 100644 --- a/src/autoscaler/scaler/testsuite/test_plug_mgr.c +++ b/src/autoscaler/scaler/testsuite/test_plug_mgr.c @@ -1,5 +1,6 @@ #include #include +#include #include "obj_mgr.h" #include "plug_mgr.h" @@ -7,7 +8,7 @@ #include "scaler.h" #include "scaler_misc.h" #include "scaler_obj.h" -#include "unity.h" +#include #define TEST_PROVIDER "test-provider" #define TEST_PROVIDER_TYPE "testProv" @@ -18,8 +19,22 @@ #define TEST_HOSTNAME "test-hostname-1" #define TEST_PRIVATE_IP "10.100.2.118" +#define ENV_PROVIDER "env-provider" + autoscaler_context_t g_scaler_ctx; +/* bug-1969: linked with -Wl,--wrap=fork (meson.build), so every fork() made + * by the scaler lands here. The scaler is multi-threaded, so a provider call + * must start its script with posix_spawn(), never fork() (tech.md "Process + * Creation"). */ +pid_t __real_fork(void); +static int fork_calls; +pid_t __wrap_fork(void) +{ + fork_calls++; + return __real_fork(); +} + extern const char* PROV_SCRIPTS[]; static void global_init(void) @@ -44,6 +59,15 @@ static void global_init(void) = strdup(PROV_SCRIPTS[GET_RETURN_REQUEST]); prov->scripts[RETURN_MACHINES] = strdup(PROV_SCRIPTS[RETURN_MACHINES]); provider_add(&g_scaler_ctx.providers, prov); + + /* a provider whose only script echoes its environment (bug-1969) */ + prov = (scaler_provider_t*)malloc(sizeof(scaler_provider_t)); + provider_init(prov, ENV_PROVIDER, TEST_PROVIDER_TYPE); + prov->confPath = strdup("/etc/xce-env-conf"); + prov->scriptPath = strdup("./testsuite/envscripts"); + prov->scripts[GET_AVAILABLE_TEMPLATES] + = strdup(PROV_SCRIPTS[GET_AVAILABLE_TEMPLATES]); + provider_add(&g_scaler_ctx.providers, prov); } static void global_cleanup(void) @@ -156,6 +180,31 @@ static void test_get_return_request(void) TEST_ASSERT_EQUAL(ALLOC_STATUS_RECLAIMED, inst->alloc_status); } +/* bug-1969: a provider call runs its script without fork(). */ +static void test_provider_call_does_not_fork(void) +{ + fork_calls = 0; + TEST_ASSERT_EQUAL_INT(0, get_available_templates(ENV_PROVIDER)); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, fork_calls, "a provider call must not fork() (bug-1969)"); +} + +/* bug-1969: the script gets the provider environment (PROVIDER_NAME, + * PRO_CONF_DIR, PRO_DATA_DIR), while the scaler's own environment is left + * alone -- it is passed to the child, not setenv() in a threaded process. */ +static void test_provider_call_passes_environment(void) +{ + char key[256]; + + TEST_ASSERT_EQUAL_INT(0, get_available_templates(ENV_PROVIDER)); + my_snprintf(key, sizeof(key), "%s#%s:%s:%s", ENV_PROVIDER, ENV_PROVIDER, + "/etc/xce-env-conf", "/tmp"); + TEST_ASSERT_NOT_NULL_MESSAGE(template_get(&g_scaler_ctx.templates, key), + "script did not see the provider environment"); + TEST_ASSERT_NULL(getenv("PROVIDER_NAME")); + TEST_ASSERT_NULL(getenv("PRO_CONF_DIR")); +} + void test_plug_mgr(void) { global_init(); @@ -166,6 +215,8 @@ void test_plug_mgr(void) RUN_TEST(test_get_return_request); RUN_TEST(test_trigger_release); RUN_TEST(test_get_release_request_status); + RUN_TEST(test_provider_call_does_not_fork); + RUN_TEST(test_provider_call_passes_environment); global_cleanup(); return; diff --git a/src/autoscaler/scaler/testsuite/test_scale_misc.c b/src/autoscaler/scaler/testsuite/test_scale_misc.c index 52791b75..114aacd7 100644 --- a/src/autoscaler/scaler/testsuite/test_scale_misc.c +++ b/src/autoscaler/scaler/testsuite/test_scale_misc.c @@ -6,7 +6,7 @@ #include "scaler/types.h" #include "scaler_misc.h" -#include "unity.h" +#include #ifndef TEST_ASSERT_NOT_EQUAL_STRING #define TEST_ASSERT_NOT_EQUAL_STRING(a, b) \