From d027410304fbc90216d81e02aa5410452a7e9eed Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 09:15:48 +0000 Subject: [PATCH] feat(job-array-one-based-index): parse_jobid on top of parse_jobsel (task 1.2) parse_jobid() keeps its signature but is now bounded and NULL-safe: a bare identity returns the exact ID, [k] the element ID, and a range, any rejected selection (5.7, 5.8, 5.14, 5.15) or a NULL argument returns -EINVAL with *jobid = 0. Fixes the strlen-before-NULL-check crash and the atoi overflow; regex parsing is gone. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 5 +- src/lib/testsuite/test_jobsel.c | 97 ++++++++++++++++++- src/lib/utils.c | 74 ++++---------- 3 files changed, 119 insertions(+), 57 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index a170e605..1816c163 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -9,7 +9,7 @@ - Done when the libxce suite runs the conformance table with every row passing, including `X[0]`, `X[01]`, the width cases (`0-0-T-1048576[1]`) and the precedence rows - _Requirements: 1.5, 5.7, 5.8, 5.12, 5.14, 5.15, 8.1_ -- [ ] 1.2 Re-implement the existing identity parse on top of the new parser +- [x] 1.2 Re-implement the existing identity parse on top of the new parser - Keep its signature; make it NULL-safe, bounded, and reject element ranges and every rejected selection with an invalid-argument result - Keep the existing format/parse round-trip tests passing unchanged - Callers outside libxce (the harness single-job commands and the scheduler REPL, whose fall-back to plain number parsing is accepted unchanged) see only the stricter acceptance @@ -236,3 +236,6 @@ - Done when both skills report PASS, the manual flow shows the expected elements and `XCE_JOBINDEX` values, and the three bug records are fixed - _Requirements: 1.1, 1.10, 5.1, 5.2, 6.1, 8.4_ - _Depends: 3.2, 7.4, 8.2, 8.3, 9.1, 9.2_ + +## Implementation Notes +- 1.2: `REGEX_JOBID`, `REGEX_JOBARRAY_ID` and `N_MAX_FIELDS_JOBID` (`src/include/config.h`) are now unused, and comments still describe regex-based `parse_jobid` in `src/scheduler/src/repl.c:143`, `src/harness/testsuite/test_test_cli.c:2324-2326` and `src/frontend/xcecli/common/format/jobid.go:86-93`; clean these up in 4.3 / 8.1 / 9.x where those files are touched. A direct `parse_jobid("...[0]")` accept test is a cheap addition when next touching `test_jobsel.c`. diff --git a/src/lib/testsuite/test_jobsel.c b/src/lib/testsuite/test_jobsel.c index efa5714f..929902cc 100644 --- a/src/lib/testsuite/test_jobsel.c +++ b/src/lib/testsuite/test_jobsel.c @@ -3,13 +3,15 @@ * * Runs the identity rows of the shared `selection-cases` conformance table * (design.md, Testing Strategy) against parse_jobsel(), plus the helpers - * is_array_tag(), jobsel_element_id() and jobsel_strerror(). + * is_array_tag(), jobsel_element_id() and jobsel_strerror(), and (task 1.2) + * parse_jobid() re-implemented on top of parse_jobsel(). * * The C surface has no decimal keys and no name selections, so the 5.17 and * name rows of the shared table do not apply here. `X` below stands for a * valid, array-capable written identity (`3-5-1700000000-65536`: sequence is * a multiple of 32768). */ +#include #include #include #include @@ -259,6 +261,96 @@ static void test_jobsel_strerror(void) TEST_ASSERT_NOT_NULL(jobsel_strerror((jobsel_err_t)99)); } +/* + * parse_jobid() (task 1.2, Requirements 5.7, 5.14, 5.15): built on + * parse_jobsel(). BARE -> the exact ID, SINGLE [k] -> the element ID; + * a range, any rejected selection, or a NULL argument -> -EINVAL with + * *jobid left at 0. + */ +static void test_parse_jobid_accepts(void) +{ + uint64_t id; + + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, X_TXT)); + TEST_ASSERT_EQUAL_UINT64(X_KEY, id); + + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, "3-5-1700000000-65541")); + TEST_ASSERT_EQUAL_UINT64(MAKE_JOBID(3, 5, 1700000000u, 0x0, 65541), id); + + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, X_TXT "[3]")); + TEST_ASSERT_EQUAL_UINT64( + MAKE_JOBID(3, 5, 1700000000u, TAG_JOB_ARRAY, 65536 | 3), id); + TEST_ASSERT_TRUE(is_array_tag(id)); + + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, X_TXT "[32767]")); + TEST_ASSERT_EQUAL_UINT64( + MAKE_JOBID(3, 5, 1700000000u, TAG_JOB_ARRAY, 65536 | 32767), id); + + /* surrounding blanks are ignored, as before */ + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, " \t" X_TXT "[2] \t")); + TEST_ASSERT_EQUAL_UINT64( + MAKE_JOBID(3, 5, 1700000000u, TAG_JOB_ARRAY, 65536 | 2), id); + + /* widest identity */ + id = 0xdead; + TEST_ASSERT_EQUAL_INT(1, parse_jobid(&id, "63-15-4294967295-1048575")); + TEST_ASSERT_EQUAL_UINT64(MAKE_JOBID(63, 15, 4294967295u, 0x0, 1048575), id); +} + +static void test_parse_jobid_rejects(void) +{ + /* clang-format off */ + static const char* const bad[] = { + X_TXT "[1-3]", /* range */ + X_TXT "[3-3]", /* range, single-valued */ + X_TXT "[32768]", /* 5.7: index above 32767 */ + X_TXT "[99999999999999999999999]",/* 5.7: saturating index */ + "3-5-1700000000-5[1]", /* 5.8: misaligned array id */ + "64-0-1700000000-5", /* 5.14: cluster too wide */ + "3-16-1700000000-5", /* 5.14: apiserver too wide */ + "3-5-4294967296-5", /* 5.14: timestamp too wide */ + "3-5-1700000000-1048576", /* 5.14: sequence too wide */ + "3-5-99999999999999999999999-5", /* 5.14: oversized field */ + X_TXT "[1]x", /* 5.15 */ + X_TXT "x", /* 5.15 */ + "x" X_TXT, /* 5.15 */ + "3-5-1700000000", /* 5.15 */ + "-3-5-1700000000-5", /* 5.15 */ + "+3-5-1700000000-5", /* 5.15 */ + X_TXT "[]", /* 5.15 */ + X_TXT " [1]", /* 5.15 */ + "12345", /* 5.15: bare decimal */ + "", /* 5.15 */ + }; + /* clang-format on */ + size_t i; + + for (i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + uint64_t id = 0xdead; + char msg[160]; + + snprintf(msg, sizeof(msg), "row %zu: \"%s\"", i, bad[i]); + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, parse_jobid(&id, (char*)bad[i]), + msg); + TEST_ASSERT_EQUAL_UINT64_MESSAGE(0, id, msg); + } +} + +static void test_parse_jobid_null(void) +{ + uint64_t id = 0xdead; + + TEST_ASSERT_EQUAL_INT(-EINVAL, parse_jobid(NULL, X_TXT)); + TEST_ASSERT_EQUAL_INT(-EINVAL, parse_jobid(NULL, NULL)); + TEST_ASSERT_EQUAL_INT(-EINVAL, parse_jobid(&id, NULL)); + TEST_ASSERT_EQUAL_UINT64(0, id); +} + void test_jobsel(void) { RUN_TEST(test_jobsel_selection_cases); @@ -267,4 +359,7 @@ void test_jobsel(void) RUN_TEST(test_jobsel_is_array_tag); RUN_TEST(test_jobsel_element_id); RUN_TEST(test_jobsel_strerror); + RUN_TEST(test_parse_jobid_accepts); + RUN_TEST(test_parse_jobid_rejects); + RUN_TEST(test_parse_jobid_null); } diff --git a/src/lib/utils.c b/src/lib/utils.c index 4c822fd9..7f5c2d4e 100644 --- a/src/lib/utils.c +++ b/src/lib/utils.c @@ -16,7 +16,6 @@ #include #endif -#include #include #include "config.h" @@ -618,72 +617,37 @@ int fmt_jobid(char* buf, size_t sz, uint64_t jobid) return fmt_jobid_ex(buf, sz, jobid, "[", "]"); } +/* + * Parse one written job identity into a job ID, on top of parse_jobsel(): + * a bare identity gives the exact ID, `[k]` gives the element ID. + * A range, any selection parse_jobsel() rejects (5.7, 5.8, 5.14, 5.15), or a + * NULL argument returns -EINVAL with *jobid (when non-NULL) set to 0. + * Returns 1 on success. + */ int parse_jobid(uint64_t* jobid, char* buf) { - regex_t regex; - regmatch_t pmatch[N_MAX_FIELDS_JOBID + 1]; - int rc; - int len = strlen(buf); + jobsel_t sel; - if ((jobid == NULL) || (buf == NULL)) { + if (jobid == NULL) { return -EINVAL; } - rc = regcomp(®ex, REGEX_JOBARRAY_ID, REG_EXTENDED); - if (rc != 0) { - return -errno; - } - *jobid = 0; - rc = regexec(®ex, buf, N_EL_ARRAY(pmatch), pmatch, 0); - if (rc == 0) { - regfree(®ex); - - if ((pmatch[0].rm_so == 0) && (pmatch[0].rm_eo == len)) { - uint32_t tag, id_cluster, id_apiserver, id_timestamp, id_job, - id_idx; - - tag = 0x01; /* job array */ - id_cluster = atoi(&buf[pmatch[1].rm_so]); - id_apiserver = atoi(&buf[pmatch[2].rm_so]); - id_timestamp = atoi(&buf[pmatch[3].rm_so]); - id_job = atoi(&buf[pmatch[4].rm_so]); - id_idx = atoi(&buf[pmatch[5].rm_so]); - - *jobid = MAKE_JOBID(id_cluster, id_apiserver, id_timestamp, tag, - id_job + id_idx); - return 1; - } + if (parse_jobsel(buf, &sel) != JOBSEL_OK) { + return -EINVAL; } - regfree(®ex); - rc = regcomp(®ex, REGEX_JOBID, REG_EXTENDED); - if (rc != 0) { - return -errno; - } - - rc = regexec(®ex, buf, N_EL_ARRAY(pmatch), pmatch, 0); - if (rc == 0) { - uint32_t id_cluster, id_apiserver, id_timestamp, id_job; - - if ((pmatch[0].rm_so != 0) || (pmatch[0].rm_eo != len)) { - regfree(®ex); - return -EINVAL; - } - - id_cluster = atoi(&buf[pmatch[1].rm_so]); - id_apiserver = atoi(&buf[pmatch[2].rm_so]); - id_timestamp = atoi(&buf[pmatch[3].rm_so]); - id_job = atoi(&buf[pmatch[4].rm_so]); - - *jobid - = MAKE_JOBID(id_cluster, id_apiserver, id_timestamp, 0x0, id_job); - - regfree(®ex); + switch (sel.form) { + case JOBSEL_BARE: + *jobid = sel.key; return 1; + case JOBSEL_SINGLE: + *jobid = jobsel_element_id(&sel, sel.lo); + return 1; + case JOBSEL_RANGE: + break; } - regfree(®ex); return -EINVAL; }