diff --git a/src/frontend/apiserver/backend/clib_test.go b/src/frontend/apiserver/backend/clib_test.go index ac21a39a..1a5f09e9 100644 --- a/src/frontend/apiserver/backend/clib_test.go +++ b/src/frontend/apiserver/backend/clib_test.go @@ -286,3 +286,20 @@ func TestClibSource_ClusterAndClientConfigsArePackageLevel(t *testing.T) { "runtime life (see status report CONCERNS for the precise finding)") } } + +// TestValidateResreqSyntax (bug-1744): the real libxce rusage grammar accepts +// well-formed requests and rejects the reported malformed one. +func TestValidateResreqSyntax(t *testing.T) { + for _, ok := range []string{"", "rusage[mem=100]", + "select[type==any] rusage[mem=100, swp=2]", "select[type==any]"} { + if err := ValidateResreqSyntax(ok); err != nil { + t.Errorf("%q: unexpected error %v", ok, err) + } + } + for _, bad := range []string{"rusage[mem=100000000a0]", + "select[type==any] rusage[mem=]", "rusage[mem=1"} { + if err := ValidateResreqSyntax(bad); err == nil { + t.Errorf("%q: expected a syntax error", bad) + } + } +} diff --git a/src/frontend/apiserver/backend/resreq_check.go b/src/frontend/apiserver/backend/resreq_check.go new file mode 100644 index 00000000..d707479e --- /dev/null +++ b/src/frontend/apiserver/backend/resreq_check.go @@ -0,0 +1,41 @@ +//go:build cgo + +package backend + +/* +#include +#include "resreq.h" +*/ +import "C" + +import ( + "fmt" + "unsafe" +) + +// ValidateResreqSyntax (bug-1744) checks every rusage[...] clause of a job's +// resource request with libxce's real rusage grammar (xce_check_rusage_syntax, +// src/lib/resreq.c) before the job is accepted. The scheduler parses the full +// request only later, asynchronously, and silently drops a job it cannot +// parse -- after the submitter was already told it was accepted. Needs no +// resource manager, so select[]/order[] resource names are not checked here. +func ValidateResreqSyntax(resreq string) error { + if resreq == "" { + return nil + } + + cs := C.CString(resreq) + defer C.free(unsafe.Pointer(cs)) + + var cerr *C.char + if rc := C.xce_check_rusage_syntax(cs, &cerr); rc < 0 { + msg := "malformed clause" + if cerr != nil { + msg = C.GoString(cerr) + C.free(unsafe.Pointer(cerr)) + } + return fmt.Errorf("invalid rusage in resource request %q: %s", + resreq, msg) + } + return nil +} diff --git a/src/frontend/apiserver/backend/stub.go b/src/frontend/apiserver/backend/stub.go index d6126195..8791bccd 100644 --- a/src/frontend/apiserver/backend/stub.go +++ b/src/frontend/apiserver/backend/stub.go @@ -46,3 +46,8 @@ func Cleanup() {} func FetchFile(_ uint64, _ string, _ uint64, _ uint32) (*FetchFileResult, error) { return nil, errNoCGO } + +// ValidateResreqSyntax (bug-1744) needs libxce's grammar parser; without CGO +// no job can be submitted anyway (SubmitJob returns errNoCGO), so there is +// nothing to check. +func ValidateResreqSyntax(string) error { return nil } diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index 9242cb07..10cde0b7 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -1530,6 +1530,10 @@ func preCheckJobRestartRequest(req *models.JobRestartRequest) error { return nil } +// validateResreqSyntax is the resource-request syntax check used by +// preCheckJobRequest; a variable so tests can substitute it (bug-1744). +var validateResreqSyntax = backend.ValidateResreqSyntax + func preCheckJobRequest(req *models.JobSubmitRequest) ([]int, error) { logger.Debug("Enter preCheckJobRequest") if req.QueueName == "" { @@ -1603,6 +1607,11 @@ func preCheckJobRequest(req *models.JobSubmitRequest) ([]int, error) { if len(req.Resreq) > 4094 { return nil, errors.New("resreq length exceeds the limit of 4094 characters") } + // bug-1744: reject a malformed rusage[] clause now, instead of + // accepting the job and letting the scheduler silently drop it. + if err := validateResreqSyntax(req.Resreq); err != nil { + return nil, err + } } if req.ShellLimit.CPULimit != 0 { diff --git a/src/frontend/apiserver/controllers/jobs_test.go b/src/frontend/apiserver/controllers/jobs_test.go index 1512df28..52d7ee48 100644 --- a/src/frontend/apiserver/controllers/jobs_test.go +++ b/src/frontend/apiserver/controllers/jobs_test.go @@ -1,6 +1,7 @@ package controllers import ( + "errors" "apiserver/models" "apiserver/store" "apiserver/utils" @@ -192,3 +193,32 @@ func TestFoundJob_MatchesRealArrayIdxType(t *testing.T) { t.Fatalf("expected to find 1 job, got %d", len(found)) } } + +// TestPreCheckJobRequest_RejectsMalformedResreq (bug-1744): a resource request +// the syntax check rejects must fail the submission instead of being accepted +// and later silently dropped by the scheduler. +func TestPreCheckJobRequest_RejectsMalformedResreq(t *testing.T) { + resetJobSubmitState() + store.Queues.Put("normal", models.QueueInfo{QueueName: "normal"}) + + var checked string + orig := validateResreqSyntax + validateResreqSyntax = func(s string) error { + checked = s + if strings.Contains(s, "100000000a0") { + return errors.New("invalid rusage in resource request") + } + return nil + } + defer func() { validateResreqSyntax = orig }() + + req := &models.JobSubmitRequest{QueueName: "normal", + Resreq: "rusage[mem=100000000a0]"} + _, err := preCheckJobRequest(req) + if checked != "rusage[mem=100000000a0]" { + t.Fatalf("resreq was not syntax-checked (checked %q)", checked) + } + if err == nil || !strings.Contains(err.Error(), "invalid rusage") { + t.Fatalf("malformed resreq must be rejected, got err=%v", err) + } +} diff --git a/src/include/resreq.h b/src/include/resreq.h index 54325025..bebda5a0 100644 --- a/src/include/resreq.h +++ b/src/include/resreq.h @@ -305,6 +305,12 @@ extern void kill_section_ctx(section_context_t*); extern void kill_resreq_data(xce_resreq_data_t*); extern int parse_section(xce_resreq_data_t*, char*, char**); extern int parse_rusage(xlist_t*, char*, char**); +/* bug-1744: syntax-check every rusage[...] clause of a resource-request + * string with the real rusage grammar -- no resource manager needed, for + * front ends validating a submission before accepting it. Returns 1 if all + * clauses parse (or there are none), else -EINVAL with *errmsg set (caller + * frees). */ +extern int xce_check_rusage_syntax(const char*, char**); extern void kill_rusage_value(rusage_value_t*); extern int parse_resreq_primary(xce_primary_resreq_t*, char*, var, char**); extern void kill_primary_resreq(xce_primary_resreq_t*); diff --git a/src/lib/resreq.c b/src/lib/resreq.c index ccd1e737..f5cc3dca 100644 --- a/src/lib/resreq.c +++ b/src/lib/resreq.c @@ -185,6 +185,68 @@ int parse_rusage(xlist_t* rusages, char* data, char** errmsg) return rc; } +/* bug-1744: see resreq.h. Mirrors section.peg's own extraction -- a clause + * runs from "rusage[" to the first "]" -- so every rusage clause of a + * primary, compound or alternative request is checked exactly as the + * scheduler's parse_resreq() will later parse it. */ +int xce_check_rusage_syntax(const char* resreq, char** errmsg) +{ + const char* p; + + if ((resreq == NULL) || (errmsg == NULL)) { + return -EINVAL; + } + *errmsg = NULL; + + for (p = strstr(resreq, "rusage["); p != NULL; + p = strstr(p, "rusage[")) { + const char* start = p + strlen("rusage["); + const char* end = strchr(start, ']'); + char* body; + const char* q; + xlist_t rusages; + int rc, i; + + if (end == NULL) { + *errmsg = strdup("unterminated rusage[ clause"); + return -EINVAL; + } + + for (q = start; (q < end) && ((*q == ' ') || (*q == '\t')); q++) + ; + if (q == end) { /* empty clause: parse_resreq_primary skips it too */ + p = end + 1; + continue; + } + + body = strndup(start, (size_t)(end - start)); + if (body == NULL) { + return -ENOMEM; + } + + xlist_init(&rusages, 4); + rc = parse_rusage(&rusages, body, errmsg); + for (i = 0; i < rusages.count; i++) { + rusage_value_t* v = (rusage_value_t*)xlist_get(&rusages, i); + + kill_rusage_value(v); + FREEUP(v); + } + xlist_fini(&rusages); + free(body); + + if (rc < 0) { + if (*errmsg == NULL) { + *errmsg = strdup("invalid rusage[] clause"); + } + return rc; + } + p = end + 1; + } + + return 1; +} + /* Depth pre-scan: count '!' and '(' as depth increments. * Returns 0 if depth is within limit, -1 if exceeded. */ static int check_select_depth(const char* input, int max_depth) diff --git a/src/lib/testsuite/test_resreq.c b/src/lib/testsuite/test_resreq.c index 94ca02c9..b82cc5ff 100644 --- a/src/lib/testsuite/test_resreq.c +++ b/src/lib/testsuite/test_resreq.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -481,11 +482,49 @@ void test_parse_section_span_not_in_select(void) FREEUP(errmsg); } +/* bug-1744: the apiserver's pre-submit check. Valid rusage clauses (and + * requests with none) pass; a malformed one -- the reported + * "rusage[mem=100000000a0]" -- is rejected with a message. */ +void test_check_rusage_syntax(void) +{ + const char* good[] = { + "rusage[mem=100]", + "select[type==any] rusage[mem=100, swp=2]", + "select[type==any] order[r15m]", + "", + "{rusage[mem=1]} || {rusage[mem=2]}", + }; + const char* bad[] = { + "rusage[mem=100000000a0]", + "select[type==any] rusage[mem=]", + "{rusage[mem=1]} || {rusage[mem=x]}", + "rusage[mem=1", + }; + char* errmsg; + size_t i; + + for (i = 0; i < sizeof(good) / sizeof(good[0]); i++) { + errmsg = NULL; + TEST_ASSERT_EQUAL_INT_MESSAGE(1, xce_check_rusage_syntax(good[i], &errmsg), + good[i]); + TEST_ASSERT_NULL(errmsg); + } + for (i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + errmsg = NULL; + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, + xce_check_rusage_syntax(bad[i], &errmsg), + bad[i]); + TEST_ASSERT_NOT_NULL_MESSAGE(errmsg, bad[i]); + free(errmsg); + } +} + void test_resreq(void) { printf("\n>> resreq test\n\n"); RUN_TEST(test_parse_section1); + RUN_TEST(test_check_rusage_syntax); RUN_TEST(test_parse_section2); RUN_TEST(test_parse_section_fail); RUN_TEST(test_parse_section3);