From f9146864a38b64d2eff3b563a8c7b596e20ba662 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 28 Sep 2026 08:49:51 +0000 Subject: [PATCH] fix(lib,scheduler): allocation constructors and allocators report OOM instead of building NULL-name records (bug-1882) make_alloc() and make_host_alloc() assigned strdup() results unchecked, so under memory exhaustion they returned a record whose resname/reserve/ host was NULL; merge_job_alloc()'s same_alloc_slot() (bug-1863) then strcmp()'d it (ASAN SEGV at xcealloc.c:337). make_alloc() also passed a NULL resname straight to strdup(). - xcealloc.c: make_alloc()/make_host_alloc() reject a NULL name (EINVAL) and check every strdup(), freeing what they built and returning NULL (ENOMEM); on failure make_host_alloc()'s caller keeps `allocs`. dup_host_alloc() frees its partial copy on failure instead of leaking. - scheduler.c sched_make_allocs(): a record that cannot be built fails the whole array (NULL, ENOMEM) instead of leaving a NULL entry. - seqjob/parjob my_alloc(): check every constructor; on failure free what was built (parjob also rolls back the remaining claims) and return -ENOMEM with ctx->alloc NULL -- the caller moves ctx.alloc onto the job whatever the rc. Tests: new xcealloc_oom_test (8, --wrap=strdup) and sched_make_allocs_oom_test (real scheduler_srcs + --wrap=strdup); a new -ENOMEM test in the seqjob and parjob allocator suites. All RED before (SEGV / "Expected NULL" / "Expected -12 Was 1"), GREEN after. xce-build.sh all asan: 115/115 suites OK, Go tests OK. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/lib/meson.build | 14 ++ src/lib/testsuite/test_xcealloc_oom.c | 210 ++++++++++++++++++ src/lib/xcealloc.c | 41 +++- src/scheduler/src/plugins/parjob_allocator.c | 32 ++- src/scheduler/src/plugins/seqjob_allocator.c | 27 ++- src/scheduler/src/scheduler.c | 8 + src/scheduler/testsuite/meson.build | 23 ++ .../testsuite/test_parjob_combinatorial.c | 65 +++++- .../testsuite/test_sched_make_allocs_oom.c | 95 ++++++++ .../testsuite/test_seqjob_allocator.c | 60 ++++- 10 files changed, 563 insertions(+), 12 deletions(-) create mode 100644 src/lib/testsuite/test_xcealloc_oom.c create mode 100644 src/scheduler/testsuite/test_sched_make_allocs_oom.c diff --git a/src/lib/meson.build b/src/lib/meson.build index 5cb9e553..d32f3070 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -241,3 +241,17 @@ libxce_test = executable('xcelib_test', install: true) test('libxce test', libxce_test, timeout: 120) + +# bug-1882 regression: `-Wl,--wrap=strdup` linker symbol interposition (same +# idiom as bug-1843's sched_record_lifecycle_test `--wrap=realloc`) so the +# tests can make a chosen strdup() inside make_alloc()/make_host_alloc() +# fail. Its own binary, so xcelib_test's other tests never see the wrap. +libxce_alloc_oom_test = executable('xcealloc_oom_test', + ['testsuite/test_xcealloc_oom.c'], + include_directories: [incdir, build_inc], + dependencies: libxce_test_deps, + c_args: ['-DXCE_TESTING'], + link_args: ['-Wl,--wrap=strdup'], + install: false) + +test('libxce alloc OOM test', libxce_alloc_oom_test, timeout: 60) diff --git a/src/lib/testsuite/test_xcealloc_oom.c b/src/lib/testsuite/test_xcealloc_oom.c new file mode 100644 index 00000000..24e053eb --- /dev/null +++ b/src/lib/testsuite/test_xcealloc_oom.c @@ -0,0 +1,210 @@ +/* + * bug-1882 regression: make_alloc()/make_host_alloc() must never return a + * record whose resname/reserve/host string is NULL. Before the fix they + * assigned strdup() results unchecked, so an allocation failure produced a + * half-built record, and merge_job_alloc()'s same_alloc_slot() later + * strcmp()'d the NULL resname (SEGV at xcealloc.c:337). + * + * `-Wl,--wrap=strdup` (lib/meson.build, same linker-interposition idiom as + * bug-1843's `--wrap=realloc` in sched_record_lifecycle_test) lets a test + * make the Nth strdup() call return NULL. libxce is a static library, so the + * wrap reaches the calls inside xcealloc.c too. __wrap_strdup() passes + * through to __real_strdup() unless a test arms it; this is a separate + * binary so xcelib_test's other tests are untouched. + */ +#include +#include +#include +#include + +#include "unity.h" +#include "xce/types.h" +#include "xcealloc.h" + +#include "controller.capnp.h" + +extern char* __real_strdup(const char* s); + +static int g_strdup_fail_at = -1; /* -1 == disabled */ +static int g_strdup_calls = 0; + +char* __wrap_strdup(const char* s) +{ + if (g_strdup_fail_at >= 0) { + int this_call = g_strdup_calls++; + + if (this_call == g_strdup_fail_at) { + return NULL; + } + } + + return __real_strdup(s); +} + +/* Make the strdup() call with 0-based index `n` (counted from now) fail. */ +static void fail_strdup_at(int n) +{ + g_strdup_calls = 0; + g_strdup_fail_at = n; +} + +void setUp(void) +{ + g_strdup_fail_at = -1; + g_strdup_calls = 0; +} + +void tearDown(void) +{ + g_strdup_fail_at = -1; +} + +static void test_make_alloc_null_resname_returns_null(void) +{ + xce_alloc_t* a; + + errno = 0; + a = make_alloc(NULL, (char*)"", 1, 1.0f, 0); + + TEST_ASSERT_NULL(a); + TEST_ASSERT_EQUAL_INT(EINVAL, errno); +} + +static void test_make_alloc_resname_strdup_failure_returns_null(void) +{ + xce_alloc_t* a; + + fail_strdup_at(0); + errno = 0; + a = make_alloc((char*)"mem", (char*)"", 1, 1.0f, 0); + + TEST_ASSERT_NULL(a); + TEST_ASSERT_EQUAL_INT(ENOMEM, errno); +} + +static void test_make_alloc_reserve_strdup_failure_returns_null(void) +{ + xce_alloc_t* a; + + fail_strdup_at(1); + errno = 0; + a = make_alloc((char*)"mem", (char*)"r1", 1, 1.0f, 0); + + TEST_ASSERT_NULL(a); + TEST_ASSERT_EQUAL_INT(ENOMEM, errno); +} + +static void test_make_alloc_default_reserve_strdup_failure_returns_null(void) +{ + xce_alloc_t* a; + + fail_strdup_at(1); /* the strdup("") for a NULL reserve */ + a = make_alloc((char*)"mem", NULL, 1, 1.0f, 0); + + TEST_ASSERT_NULL(a); +} + +static void test_make_alloc_success_sets_every_string(void) +{ + xce_alloc_t* a = make_alloc((char*)"mem", NULL, 1, 1.0f, 0); + + TEST_ASSERT_NOT_NULL(a); + TEST_ASSERT_EQUAL_STRING("mem", a->resname); + TEST_ASSERT_EQUAL_STRING("", a->reserve); + + free_ResAlloc_ptr(&a); +} + +static void test_make_host_alloc_null_host_returns_null(void) +{ + xce_host_alloc_t* h; + + errno = 0; + h = make_host_alloc(NULL, 0, NULL); + + TEST_ASSERT_NULL(h); + TEST_ASSERT_EQUAL_INT(EINVAL, errno); +} + +/* On failure the caller keeps ownership of `allocs`: freeing it afterwards + * must be neither a double free nor a leak (ASAN checks both). */ +static void test_make_host_alloc_strdup_failure_leaves_allocs_to_caller(void) +{ + xce_alloc_t** allocs; + xce_host_alloc_t* h; + + allocs = make_allocs(1, make_alloc((char*)"mem", NULL, 1, 1.0f, 0)); + TEST_ASSERT_NOT_NULL(allocs); + + fail_strdup_at(0); + errno = 0; + h = make_host_alloc((char*)"h0", 1, allocs); + + TEST_ASSERT_NULL(h); + TEST_ASSERT_EQUAL_INT(ENOMEM, errno); + + free_ResAlloc_list(1, allocs); +} + +static xce_job_alloc_t* one_host_job(const char* host) +{ + xce_alloc_t** allocs; + xce_host_alloc_t** hallocs; + + allocs = make_allocs(1, make_alloc((char*)"mem", NULL, 1, 1.0f, 0)); + hallocs = make_host_allocs(1, make_host_alloc((char*)host, 1, allocs)); + + return make_job_alloc(7, 1, hallocs); +} + +/* merge_job_alloc() deep-copies a new src host via dup_host_alloc(), which + * builds it with make_alloc()/make_host_alloc(): a failed strdup() there must + * surface as -ENOMEM, never as a host or resource with a NULL name left in + * dst for the next merge to strcmp(). */ +static void test_merge_job_alloc_strdup_failure_reports_enomem(void) +{ + int n; + + /* strdup() calls when copying one new host with one resource: + * 0 = resname, 1 = reserve, 2 = host. Fail each in turn. */ + for (n = 0; n < 3; n++) { + xce_job_alloc_t* dst = one_host_job("h0"); + xce_job_alloc_t* src = one_host_job("h1"); + int rc; + int i; + + TEST_ASSERT_NOT_NULL(dst); + TEST_ASSERT_NOT_NULL(src); + + fail_strdup_at(n); + rc = merge_job_alloc(dst, src); + g_strdup_fail_at = -1; + + TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOMEM, rc, "merge must report OOM"); + for (i = 0; i < dst->nhosts; i++) { + TEST_ASSERT_NOT_NULL(dst->allocs[i]); + TEST_ASSERT_NOT_NULL(dst->allocs[i]->host); + } + + free_JobAlloc_ptr(&dst); + free_JobAlloc_ptr(&src); + } +} + +int main(void) +{ + UNITY_BEGIN(); + + printf("\n>> xcealloc OOM test (bug-1882)\n\n"); + + RUN_TEST(test_make_alloc_null_resname_returns_null); + RUN_TEST(test_make_alloc_resname_strdup_failure_returns_null); + RUN_TEST(test_make_alloc_reserve_strdup_failure_returns_null); + RUN_TEST(test_make_alloc_default_reserve_strdup_failure_returns_null); + RUN_TEST(test_make_alloc_success_sets_every_string); + RUN_TEST(test_make_host_alloc_null_host_returns_null); + RUN_TEST(test_make_host_alloc_strdup_failure_leaves_allocs_to_caller); + RUN_TEST(test_merge_job_alloc_strdup_failure_reports_enomem); + + return UNITY_END(); +} diff --git a/src/lib/xcealloc.c b/src/lib/xcealloc.c index 2de73373..4a4724be 100644 --- a/src/lib/xcealloc.c +++ b/src/lib/xcealloc.c @@ -8,21 +8,34 @@ #include "xce/types.h" #include "xcealloc.h" +/* make_alloc() never returns a record with a NULL resname or reserve + * (bug-1882): a NULL resname is rejected (errno EINVAL) and a failed + * strdup() frees what was built and returns NULL (errno ENOMEM), because + * merge_job_alloc()'s slot matching and every other consumer strcmp() these + * strings. */ xce_alloc_t* make_alloc(char* resname, char* reserve, uint64_t instid, float value, int duration) { xce_alloc_t* alloc = NULL; + if (resname == NULL) { + errno = EINVAL; + return NULL; + } + alloc = (xce_alloc_t*)calloc(1, sizeof(*alloc)); if (alloc == NULL) { return NULL; } alloc->resname = strdup(resname); - if (reserve != NULL) { - alloc->reserve = strdup(reserve); - } else { - alloc->reserve = strdup(""); + alloc->reserve = strdup((reserve != NULL) ? reserve : ""); + if ((alloc->resname == NULL) || (alloc->reserve == NULL)) { + FREEUP(alloc->resname); + FREEUP(alloc->reserve); + FREEUP(alloc); + errno = ENOMEM; + return NULL; } alloc->instid = instid; @@ -78,18 +91,32 @@ xce_alloc_t** make_allocs(int nallocs, ...) return allocs; } +/* make_host_alloc() takes ownership of `allocs` only on success; on failure + * (NULL host: EINVAL, failed strdup(): ENOMEM) it returns NULL and the + * caller still owns `allocs` (bug-1882: never a record with a NULL host). */ xce_host_alloc_t* make_host_alloc(char* host, int nalloc, xce_alloc_t** allocs) { xce_host_alloc_t* halloc = NULL; + if (host == NULL) { + errno = EINVAL; + return NULL; + } + halloc = (xce_host_alloc_t*)calloc(1, sizeof(*halloc)); if (halloc == NULL) { return NULL; } + halloc->host = strdup(host); + if (halloc->host == NULL) { + FREEUP(halloc); + errno = ENOMEM; + return NULL; + } + halloc->nallocs = nalloc; halloc->allocs = allocs; - halloc->host = strdup(host); return halloc; } @@ -219,12 +246,16 @@ static xce_host_alloc_t* dup_host_alloc(const xce_host_alloc_t* src) for (i = 0; i < src->nallocs; i++) { allocs[i] = dup_alloc(src->allocs[i]); if (allocs[i] == NULL) { + free_ResAlloc_list(src->nallocs, allocs); return NULL; } } } halloc = make_host_alloc(src->host, src->nallocs, allocs); + if (halloc == NULL) { + free_ResAlloc_list(src->nallocs, allocs); + } return halloc; } diff --git a/src/scheduler/src/plugins/parjob_allocator.c b/src/scheduler/src/plugins/parjob_allocator.c index ace0a58b..c880a4ff 100644 --- a/src/scheduler/src/plugins/parjob_allocator.c +++ b/src/scheduler/src/plugins/parjob_allocator.c @@ -690,20 +690,41 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) return -ENOMEM; } + /* bug-1882: any constructor failing (OOM) is -ENOMEM with + * ctx->alloc left NULL (the caller moves ctx->alloc onto the + * job whatever the rc): free the host records built so far, + * roll back the claims not yet turned into one. */ for (i = 0; i < nclaimed; i++) { + int nres = claimed[i].host_req.count; xce_alloc_t** allocs - = sched_make_allocs(sched, "", claimed[i].host_req.count, + = sched_make_allocs(sched, "", nres, (int*)claimed[i].host_req.idxs.data, (float*)claimed[i].host_req.vals.data, /* FIXME: no duration now */ NULL); - hallocs[i] = make_host_alloc(claimed[i].w->name, - claimed[i].host_req.count, allocs); + if ((allocs != NULL) || (nres <= 0)) { + hallocs[i] + = make_host_alloc(claimed[i].w->name, nres, allocs); + if (hallocs[i] == NULL) { + free_ResAlloc_list(nres, allocs); + } + } + if (hallocs[i] == NULL) { + parjob_rollback_claims_(ctx, claimed + i, nclaimed - i); + free_HostAlloc_list(nclaimed, hallocs); + plan_iter->fini(plan_iter); + return -ENOMEM; + } xvec_idval_fini(&(claimed[i].host_req)); } ctx->alloc = make_job_alloc(0, nclaimed, hallocs); + if (ctx->alloc == NULL) { + free_HostAlloc_list(nclaimed, hallocs); + plan_iter->fini(plan_iter); + return -ENOMEM; + } if (ctx->alloc_req.shared.count > 0) { ctx->alloc->allocs_shared @@ -711,6 +732,11 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) ctx->alloc_req.shared.idxs.data, ctx->alloc_req.shared.vals.data, /* FIXME: no duration now */ NULL); + if (ctx->alloc->allocs_shared == NULL) { + free_JobAlloc_ptr(&(ctx->alloc)); + plan_iter->fini(plan_iter); + return -ENOMEM; + } ctx->alloc->nres_shared = ctx->alloc_req.shared.count; } else { ctx->alloc->allocs_shared = NULL; diff --git a/src/scheduler/src/plugins/seqjob_allocator.c b/src/scheduler/src/plugins/seqjob_allocator.c index 433a67db..329cf80f 100644 --- a/src/scheduler/src/plugins/seqjob_allocator.c +++ b/src/scheduler/src/plugins/seqjob_allocator.c @@ -362,14 +362,35 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) xce_host_alloc_t* halloc = NULL; xce_alloc_t** allocs = NULL; + /* bug-1882: any constructor failing (OOM) is -ENOMEM with + * ctx->alloc left NULL -- the caller moves ctx->alloc onto the job + * whatever the rc, so a half-built allocation must never stay + * there. The debits this attempt made live in ctx's scratch and + * are dropped by sched_ctx_fini(). */ allocs = sched_make_allocs( sched, "", ctx->alloc_req.host.count, ctx->alloc_req.host.idxs.data, ctx->alloc_req.host.vals.data, /* FIXME: no duration now */ NULL); + if ((allocs == NULL) && (ctx->alloc_req.host.count > 0)) { + return -ENOMEM; + } + + halloc = make_host_alloc(w->name, ctx->alloc_req.host.count, allocs); + if (halloc == NULL) { + free_ResAlloc_list(ctx->alloc_req.host.count, allocs); + return -ENOMEM; + } - halloc = make_host_alloc(w->name, ctx->alloc_req.host.count, allocs); hallocs = make_host_allocs(1, halloc); + if (hallocs == NULL) { + free_HostAlloc_ptr(&halloc); + return -ENOMEM; + } ctx->alloc = make_job_alloc(0, 1, hallocs); + if (ctx->alloc == NULL) { + free_HostAlloc_list(1, hallocs); + return -ENOMEM; + } if (ctx->alloc_req.shared.count > 0) { ctx->alloc->allocs_shared @@ -377,6 +398,10 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) ctx->alloc_req.shared.idxs.data, ctx->alloc_req.shared.vals.data, /*FIXME: no duration now*/ NULL); + if (ctx->alloc->allocs_shared == NULL) { + free_JobAlloc_ptr(&(ctx->alloc)); + return -ENOMEM; + } ctx->alloc->nres_shared = ctx->alloc_req.shared.count; } else { ctx->alloc->allocs_shared = NULL; diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index f2281f27..cc993481 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -1891,6 +1891,14 @@ xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, int nres, allocs[i] = make_alloc(r->name, reserve, sched_get_id(sched), vals[i], d); + if (allocs[i] == NULL) { + /* bug-1882: a record that cannot be built fails the whole + * array -- a NULL entry would reach the allocator plugins as a + * host allocation silently missing this resource. */ + free_ResAlloc_list(nres, allocs); + errno = ENOMEM; + return NULL; + } } return allocs; diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 5f2b174d..ade18318 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -1917,6 +1917,29 @@ test('scheduler repl resource formatting test', test('scheduler connect pipeline test', sched_scheduler_connect_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) + +# bug-1882 regression: sched_make_allocs() must fail the whole array when +# make_alloc() cannot build a record. Real scheduler_srcs (as +# sched_scheduler_connect_test) plus `-Wl,--wrap=strdup` to make a chosen +# strdup() inside make_alloc() fail. +sched_make_allocs_oom_test_srcs = ['test_sched_make_allocs_oom.c'] +foreach f : scheduler_srcs + if f != 'src/main.c' + sched_make_allocs_oom_test_srcs += ['../' + f] + endif +endforeach + +sched_make_allocs_oom_test = executable( + 'sched_make_allocs_oom_test', + sched_make_allocs_oom_test_srcs, + include_directories:[incdir], + dependencies: [scheduler_deps, unity_dep], + link_args: ['-Wl,--wrap=strdup'], + install: false) + +test('scheduler make_allocs OOM test', + sched_make_allocs_oom_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) test('scheduler decision publish retry test', sched_scheduler_decision_publish_retry_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_parjob_combinatorial.c b/src/scheduler/testsuite/test_parjob_combinatorial.c index fcb6bed8..e9ffb851 100644 --- a/src/scheduler/testsuite/test_parjob_combinatorial.c +++ b/src/scheduler/testsuite/test_parjob_combinatorial.c @@ -182,6 +182,11 @@ int plugin_job_rusage_update(void* s, xce_sched_job_t* j, xce_job_rusages_t* r) * assert_uncommitted_host_debit() (real ctx->uncomitted state, untouched * by this stub) rather than these fake xce_alloc_t's fields, exactly like * the seqjob equivalence tests already do. */ +/* bug-1882: arm to make the stub's Nth call (0-based) fail like the real + * sched_make_allocs() does when make_alloc() cannot build a record. */ +static int g_make_allocs_fail_at = -1; /* -1 == disabled */ +static int g_make_allocs_calls = 0; + xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, int nres, int* idxs, float* vals, int* duration) { @@ -194,6 +199,12 @@ xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, int nres, (void)vals; (void)duration; + if ((g_make_allocs_fail_at >= 0) + && (g_make_allocs_calls++ == g_make_allocs_fail_at)) { + errno = ENOMEM; + return NULL; + } + if (nres <= 0) { return NULL; } @@ -730,6 +741,53 @@ static void test_multi_bucket_plan_succeeds_through_real_my_alloc(void) sched_ctx_fini(&ctx); } +/* + * bug-1882: a two-host plan whose allocation records cannot be built -- + * first for host 0, then for host 1 after host 0's record already exists -- + * must report -ENOMEM with ctx->alloc left NULL (the caller moves ctx->alloc + * onto the job whatever the rc). ASAN catches a double free or use after + * free on the cleanup path. + */ +static void test_my_alloc_reports_enomem_when_allocs_cannot_be_built(void) +{ + int fail_at; + + for (fail_at = 0; fail_at < 2; fail_at++) { + xce_scheduler_t sched; + xce_resource_t* slots_res; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary_obj; + xce_primary_resreq_t primary; + xce_sched_ctx_t ctx; + int rc; + + memset(&sched, 0, sizeof(sched)); + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core))); + + slots_res = register_slots_resource(sched.core.resmgr); + TEST_ASSERT_NOT_NULL(slots_res); + add_worker_with_slots(&(sched.core), "host-a", slots_res->idx, 2.0f); + add_worker_with_slots(&(sched.core), "host-b", slots_res->idx, 1.0f); + + memset(&primary, 0, sizeof(primary)); + primary.minTasks = 1; + primary.maxTasks = 3; + primary.select = NULL; + + init_primary_obj(&obj, &primary_obj, &primary); + + g_make_allocs_calls = 0; + g_make_allocs_fail_at = fail_at; + rc = drive_my_alloc(&sched, &obj, &primary, &ctx); + g_make_allocs_fail_at = -1; + + TEST_ASSERT_EQUAL_INT(-ENOMEM, rc); + TEST_ASSERT_NULL(ctx.alloc); + + sched_ctx_fini(&ctx); + } +} + /* * Req 4 AC 2: the exact descending-vs-ascending worked example from * requirements.md's Composition Algorithm. Two hosts, capacities 1 and 2; @@ -1157,7 +1215,11 @@ static void test_shared_resource_debited_once_regardless_of_host_count(void) sched_ctx_fini(&ctx); } -void setUp(void) { return; } +void setUp(void) +{ + g_make_allocs_fail_at = -1; + g_make_allocs_calls = 0; +} void tearDown(void) { return; } @@ -1171,6 +1233,7 @@ int main(void) RUN_TEST(test_init_rejects_invalid_input); RUN_TEST(test_next_has_a_real_cancellation_point); RUN_TEST(test_multi_bucket_plan_succeeds_through_real_my_alloc); + RUN_TEST(test_my_alloc_reports_enomem_when_allocs_cannot_be_built); RUN_TEST(test_descending_bucket_order_succeeds_where_ascending_fails); RUN_TEST( test_mid_plan_bucket_failure_rolls_back_before_later_plan_succeeds); diff --git a/src/scheduler/testsuite/test_sched_make_allocs_oom.c b/src/scheduler/testsuite/test_sched_make_allocs_oom.c new file mode 100644 index 00000000..af0aee1c --- /dev/null +++ b/src/scheduler/testsuite/test_sched_make_allocs_oom.c @@ -0,0 +1,95 @@ +/* + * bug-1882 regression, scheduler side: sched_make_allocs() (scheduler.c) + * must return NULL when make_alloc() cannot build one of the records, + * instead of an array with a NULL entry that the allocator plugins would + * hand on as a host allocation silently missing a resource. + * + * Links the real scheduler_srcs (same technique as + * sched_scheduler_connect_test) plus `-Wl,--wrap=strdup` (bug-1843's + * `--wrap=realloc` idiom), so a chosen strdup() inside make_alloc() fails. + */ +#include +#include +#include + +#include + +#include "resobj.h" +#include "scheduler.h" +#include "xcealloc.h" + +extern char* __real_strdup(const char* s); + +static int g_strdup_fail_at = -1; /* -1 == disabled */ +static int g_strdup_calls = 0; + +char* __wrap_strdup(const char* s) +{ + if (g_strdup_fail_at >= 0) { + int this_call = g_strdup_calls++; + + if (this_call == g_strdup_fail_at) { + return NULL; + } + } + + return __real_strdup(s); +} + +void setUp(void) +{ + g_strdup_fail_at = -1; + g_strdup_calls = 0; +} + +void tearDown(void) +{ + g_strdup_fail_at = -1; +} + +static void test_sched_make_allocs_returns_null_when_a_record_fails(void) +{ + xce_scheduler_t sched; + xce_resource_t res; + xce_resource_t* slots; + xce_alloc_t** allocs; + int idxs[2]; + float vals[2] = { 1.0f, 2.0f }; + + memset(&sched, 0, sizeof(sched)); + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core))); + + memset(&res, 0, sizeof(res)); + res.name = strdup("slots"); + slots = makeResource(sched.core.resmgr, &res); + TEST_ASSERT_NOT_NULL(slots); + idxs[0] = slots->idx; + idxs[1] = slots->idx; + + /* Control: both records built. */ + allocs = sched_make_allocs(&sched, (char*)"", 2, idxs, vals, NULL); + TEST_ASSERT_NOT_NULL(allocs); + TEST_ASSERT_NOT_NULL(allocs[0]); + TEST_ASSERT_NOT_NULL(allocs[1]); + free_ResAlloc_list(2, allocs); + + /* strdup() calls: record 0 resname = 0, reserve = 1; record 1 + * resname = 2. Fail record 1's resname. */ + g_strdup_calls = 0; + g_strdup_fail_at = 2; + errno = 0; + allocs = sched_make_allocs(&sched, (char*)"", 2, idxs, vals, NULL); + g_strdup_fail_at = -1; + + TEST_ASSERT_NULL_MESSAGE(allocs, + "a record that cannot be built must fail the " + "whole array, not leave a NULL entry"); + TEST_ASSERT_EQUAL_INT(ENOMEM, errno); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_sched_make_allocs_returns_null_when_a_record_fails); + return UNITY_END(); +} diff --git a/src/scheduler/testsuite/test_seqjob_allocator.c b/src/scheduler/testsuite/test_seqjob_allocator.c index 42a80d6c..cac09463 100644 --- a/src/scheduler/testsuite/test_seqjob_allocator.c +++ b/src/scheduler/testsuite/test_seqjob_allocator.c @@ -134,6 +134,10 @@ int plugin_job_rusage_update(void* s, xce_sched_job_t* j, xce_job_rusages_t* r) * how many resource entries it carries, not the internal xce_alloc_t * field contents make_alloc() (xcealloc.c, unmodified, real) would fill * in for a live cluster. */ +/* bug-1882: set to make the stub fail like the real sched_make_allocs() + * does when make_alloc() cannot build a record. */ +static int g_make_allocs_fail = 0; + xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, int nres, int* idxs, float* vals, int* duration) @@ -147,6 +151,11 @@ xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, (void)vals; (void)duration; + if (g_make_allocs_fail) { + errno = ENOMEM; + return NULL; + } + if (nres <= 0) { return NULL; } @@ -163,8 +172,8 @@ xce_alloc_t** sched_make_allocs(xce_scheduler_t* sched, char* reserve, return allocs; } -void setUp(void) { return; } -void tearDown(void) { return; } +void setUp(void) { g_make_allocs_fail = 0; } +void tearDown(void) { g_make_allocs_fail = 0; } /* ---------------------------------------------------------------------- */ /* Fixture helpers */ @@ -379,9 +388,56 @@ test_seqjob_prepare_allocation_guards_against_maxtasks_gt_1_fanout(void) sched_ctx_fini(&ctx); } +/* + * bug-1882: when the allocation records cannot be built (sched_make_allocs() + * returns NULL), my_alloc() must report -ENOMEM and leave ctx->alloc NULL + * -- sched_make_primary_allocation() moves ctx.alloc onto the job whatever + * the rc, so a half-built allocation there would be dispatched. + */ +static void test_my_alloc_reports_enomem_when_allocs_cannot_be_built(void) +{ + xce_scheduler_t sched; + xce_resource_t* slots_res; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary_obj; + xce_primary_resreq_t primary; + xce_sched_ctx_t ctx; + xce_sched_plugin_t plugin; + int rc; + + memset(&sched, 0, sizeof(sched)); + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core))); + + slots_res = register_slots_resource(sched.core.resmgr); + TEST_ASSERT_NOT_NULL(slots_res); + add_worker_with_slots(&(sched.core), WORKER_NAME, slots_res->idx, 4.0f); + + memset(&primary, 0, sizeof(primary)); + primary.minTasks = 1; + primary.maxTasks = 1; + primary.select = NULL; + + init_primary_obj(&obj, &primary_obj, &primary); + TEST_ASSERT_EQUAL_INT(0, sched_ctx_init(&ctx, &(sched.core), &obj)); + sched_ctx_set_primary(&ctx, &primary); + + memset(&plugin, 0, sizeof(plugin)); + plugin.sched = &sched; + plugin.core = &(sched.core); + + g_make_allocs_fail = 1; + rc = my_alloc(&plugin, &ctx); + + TEST_ASSERT_EQUAL_INT(-ENOMEM, rc); + TEST_ASSERT_NULL(ctx.alloc); + + sched_ctx_fini(&ctx); +} + int main(void) { UNITY_BEGIN(); + RUN_TEST(test_my_alloc_reports_enomem_when_allocs_cannot_be_built); RUN_TEST( test_my_alloc_allocates_single_matching_host_through_shared_iterators); RUN_TEST(test_seqjob_prepare_allocation_guards_against_maxtasks_gt_1_fanout);