diff --git a/src/scheduler/src/repl_break.c b/src/scheduler/src/repl_break.c index 92742701..4ddc476d 100644 --- a/src/scheduler/src/repl_break.c +++ b/src/scheduler/src/repl_break.c @@ -548,12 +548,28 @@ bool repl_cmd_delete(xce_sched_sim_driver_t* sim, const char* line) } { - int id = atoi(tokens[1]); + /* strtol()+endptr, not atoi() -- same "fail loud" discipline + * repl.c's repl_cmd_step() doc comment establishes: atoi() would + * silently coerce a non-numeric token (e.g. a typo'd id) to 0 + * rather than reporting the input as invalid, misreporting a bad + * argument as "no breakpoint with id 0". Breakpoint ids are + * assigned via pre-incremented next_bp_id (repl_cmd_break(), this + * file), so 1 is the smallest id that can ever legitimately exist. */ + long id; + char* endptr; - if (!repl_breakpoint_remove(sim->repl_state, id)) { - fprintf(stdout, "[sim] delete: no breakpoint with id %d\n", id); + id = strtol(tokens[1], &endptr, 10); + if ((endptr == tokens[1]) || (*endptr != '\0') || (id <= 0)) { + fprintf(stdout, + "[sim] delete: '%s' is not a valid breakpoint id -- usage: " + "delete |all\n", tokens[1]); + return false; + } + + if (!repl_breakpoint_remove(sim->repl_state, (int)id)) { + fprintf(stdout, "[sim] delete: no breakpoint with id %ld\n", id); } else { - fprintf(stdout, "[sim] breakpoint %d deleted\n", id); + fprintf(stdout, "[sim] breakpoint %ld deleted\n", id); } } diff --git a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c index 8a60b8a2..4f9e8ba9 100644 --- a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c +++ b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c @@ -420,6 +420,44 @@ static void test_delete_all_removes_all_breakpoints(void) printf("test_delete_all_removes_all_breakpoints: PASS\n"); } +static void test_delete_non_numeric_id_rejected(void) +{ + xce_sched_sim_driver_t sim; + + memset(&sim, 0, sizeof(sim)); + /* xce_sched_core_init() is the same precondition xce_repl_start() + * always has satisfied for it in the real driver flow (sched_sim_driver_start() + * runs first) -- it also transitively calls xstring_table_init() + * (core.c), which makeString() (breakpoint token/filter string copies, + * repl_break.c) requires to not silently return NULL. */ + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sim.core))); + sim.repl_state = repl_state_ensure(&sim); + TEST_ASSERT_NOT_NULL(sim.repl_state); + + TEST_ASSERT_FALSE(repl_dispatch(&sim, "break JOB_PUSH")); + TEST_ASSERT_EQUAL_INT(1, sim.repl_state->nbreakpoints); + + /* "abc" must be rejected outright, not silently coerced to id 0 + * (atoi()'s old behavior) and reported as "no breakpoint with id 0". */ + TEST_ASSERT_FALSE(repl_dispatch(&sim, "delete abc")); + TEST_ASSERT_EQUAL_INT(1, sim.repl_state->nbreakpoints); + + /* "0" and negative ids can never be legitimately assigned + * (repl_cmd_break()'s pre-incremented next_bp_id starts at 1) -- also + * rejected as invalid, not treated as a well-formed-but-missing id. */ + TEST_ASSERT_FALSE(repl_dispatch(&sim, "delete 0")); + TEST_ASSERT_EQUAL_INT(1, sim.repl_state->nbreakpoints); + TEST_ASSERT_FALSE(repl_dispatch(&sim, "delete -1")); + TEST_ASSERT_EQUAL_INT(1, sim.repl_state->nbreakpoints); + + /* trailing garbage after a numeric prefix ("1x") is also rejected, not + * truncated down to id 1. */ + TEST_ASSERT_FALSE(repl_dispatch(&sim, "delete 1x")); + TEST_ASSERT_EQUAL_INT(1, sim.repl_state->nbreakpoints); + + printf("test_delete_non_numeric_id_rejected: PASS\n"); +} + static void test_info_break_lists_breakpoints_with_filters(void) { xce_sched_sim_driver_t sim; @@ -1997,6 +2035,7 @@ int main(void) RUN_TEST(test_break_malformed_filter_rejected); RUN_TEST(test_delete_by_id_removes_one_breakpoint); RUN_TEST(test_delete_all_removes_all_breakpoints); + RUN_TEST(test_delete_non_numeric_id_rejected); RUN_TEST(test_info_break_lists_breakpoints_with_filters); RUN_TEST(test_event_breakpoint_matches_jobid_filter);