From 030a08092aa9121da14b538512094088841b2ac0 Mon Sep 17 00:00:00 2001 From: HASUMI Hitoshi Date: Thu, 28 May 2026 18:13:18 +0900 Subject: [PATCH 1/4] Separate the union of timeslice and result in struct mrb_task Bug scenario: * VM returns an Exception `t->state.result = mrb_vm_exec(...);` * Despite task is still MRB_TASK_STATUS_RUNNING, IRQ triggered by chance and `mrb_tick()` executes `t->state.timeslice--;` * But the same memory area already holds the `result`, `timeslice--` reduces `result.value.p`'s top byte The fix is to separate `timeslice` and `result` into different fields. I have considered improving critical sections, but I ended up with this patch because I believe it is widely effective and less error-prone. --- mrbgems/mruby-task/include/task.h | 8 ++------ mrbgems/mruby-task/src/task.c | 24 ++++++++++++------------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/mrbgems/mruby-task/include/task.h b/mrbgems/mruby-task/include/task.h index 0719fe018..d9c458629 100644 --- a/mrbgems/mruby-task/include/task.h +++ b/mrbgems/mruby-task/include/task.h @@ -41,7 +41,6 @@ struct mrb_task_queue; * - Removed started flag (inferred from c.status): 1 byte * - Unified wakeup_tick/join/mutex into single union: 4 bytes * - Removed redundant proc field (stored in c.ci->proc): 8 bytes - * - Unified timeslice/result into state union: ~4 bytes * Total savings: ~18 bytes per task (14% reduction) */ typedef struct mrb_task { @@ -61,11 +60,8 @@ typedef struct mrb_task { mrb_value self; /* Ruby Task object reference */ - /* State-specific data - mutually exclusive based on status */ - union { - volatile uint8_t timeslice; /* Remaining ticks (RUNNING only) */ - mrb_value result; /* Task return value (DORMANT only) */ - } state; + volatile uint8_t timeslice; /* Remaining ticks while RUNNING */ + mrb_value result; /* Task return value */ struct mrb_context c; /* Execution context (stack, callinfo, etc) */ } mrb_task; diff --git a/mrbgems/mruby-task/src/task.c b/mrbgems/mruby-task/src/task.c index a94241534..2b91f5c65 100644 --- a/mrbgems/mruby-task/src/task.c +++ b/mrbgems/mruby-task/src/task.c @@ -114,7 +114,7 @@ mrb_task_mark_all(mrb_state *mrb) /* Mark task-specific values */ mrb_gc_mark_value(mrb, t->self); if (t->status == MRB_TASK_STATUS_DORMANT) { - mrb_gc_mark_value(mrb, t->state.result); + mrb_gc_mark_value(mrb, t->result); } mrb_gc_mark_value(mrb, t->name); @@ -326,7 +326,7 @@ execute_task(mrb_state *mrb, mrb_task *t) /* Set task as running */ t->status = MRB_TASK_STATUS_RUNNING; - t->state.timeslice = MRB_TIMESLICE_TICK_COUNT; + t->timeslice = MRB_TIMESLICE_TICK_COUNT; /* Switch to task context */ prev_c = mrb->c; @@ -351,7 +351,7 @@ execute_task(mrb_state *mrb, mrb_task *t) t->c.vmexec = TRUE; /* Execute task - PC is saved in ci->pc from previous run */ - t->state.result = mrb_vm_exec(mrb, proc, pc); + t->result = mrb_vm_exec(mrb, proc, pc); /* Clear vmexec flag */ t->c.vmexec = FALSE; @@ -394,9 +394,9 @@ mrb_tick(mrb_state *mrb) /* Decrease timeslice for running task */ t = q_ready_; - if (t && t->status == MRB_TASK_STATUS_RUNNING && t->state.timeslice > 0) { - t->state.timeslice--; - if (t->state.timeslice == 0) { + if (t && t->status == MRB_TASK_STATUS_RUNNING && t->timeslice > 0) { + t->timeslice--; + if (t->timeslice == 0) { switching_ = TRUE; /* Trigger context switch */ } } @@ -1115,7 +1115,7 @@ mrb_task_join(mrb_state *mrb, mrb_value self) /* If task is already dormant, return immediately */ if (t->status == MRB_TASK_STATUS_DORMANT) { - return t->state.result; + return t->result; } /* Wait for task to complete */ @@ -1130,7 +1130,7 @@ mrb_task_join(mrb_state *mrb, mrb_value self) /* Trigger context switch */ switching_ = TRUE; - return t->state.result; + return t->result; } /* @@ -1190,16 +1190,16 @@ mrb_execute_proc_synchronously(mrb_state *mrb, mrb_value proc_val, mrb_int argc, mrb->c = &t->c; while (t->c.status != MRB_TASK_STOPPED) { - t->state.result = mrb_vm_exec(mrb, mrb->c->ci->proc, mrb->c->ci->pc); + t->result = mrb_vm_exec(mrb, mrb->c->ci->proc, mrb->c->ci->pc); } /* If there's an unhandled exception after VM stops, save it as result */ if (mrb->exc) { - t->state.result = mrb_obj_value(mrb->exc); + t->result = mrb_obj_value(mrb->exc); } /* 5. Get result and clean up */ - mrb_value result = t->state.result; + mrb_value result = t->result; if (mrb_obj_ptr(result) == mrb->exc) { mrb->exc = NULL; /* Clear exception */ } @@ -1437,7 +1437,7 @@ mrb_task_value(mrb_state *mrb, mrb_value task) mrb_task *t = (mrb_task*)mrb_data_check_get_ptr(mrb, task, &mrb_task_type); if (!t) return mrb_nil_value(); - return t->state.result; + return t->result; } /* From 5e669560ebee9169f1b1d3ce2c7fcc50bbb92cc6 Mon Sep 17 00:00:00 2001 From: HASUMI Hitoshi Date: Thu, 28 May 2026 19:07:20 +0900 Subject: [PATCH 2/4] Amend field position to pack effectively --- mrbgems/mruby-task/include/task.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mrbgems/mruby-task/include/task.h b/mrbgems/mruby-task/include/task.h index d9c458629..6a464eb9c 100644 --- a/mrbgems/mruby-task/include/task.h +++ b/mrbgems/mruby-task/include/task.h @@ -48,6 +48,7 @@ typedef struct mrb_task { uint8_t priority; /* Priority (0-255, 0=highest) */ uint8_t status; /* Current status (TASKSTATUS enum) */ uint8_t reason; /* Wait reason (TASKREASON enum) */ + volatile uint8_t timeslice; /* Remaining ticks while RUNNING */ mrb_value name; /* Optional task name */ /* Wait-specific data - mutually exclusive based on reason field */ @@ -60,7 +61,6 @@ typedef struct mrb_task { mrb_value self; /* Ruby Task object reference */ - volatile uint8_t timeslice; /* Remaining ticks while RUNNING */ mrb_value result; /* Task return value */ struct mrb_context c; /* Execution context (stack, callinfo, etc) */ From 564726c44f7d4df3efeff749a18f62e444967ee2 Mon Sep 17 00:00:00 2001 From: HASUMI Hitoshi Date: Thu, 28 May 2026 19:16:04 +0900 Subject: [PATCH 3/4] t->result now can be marked unconditionally --- mrbgems/mruby-task/src/task.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/mrbgems/mruby-task/src/task.c b/mrbgems/mruby-task/src/task.c index 2b91f5c65..fbca3ceec 100644 --- a/mrbgems/mruby-task/src/task.c +++ b/mrbgems/mruby-task/src/task.c @@ -113,9 +113,7 @@ mrb_task_mark_all(mrb_state *mrb) /* Mark task-specific values */ mrb_gc_mark_value(mrb, t->self); - if (t->status == MRB_TASK_STATUS_DORMANT) { - mrb_gc_mark_value(mrb, t->result); - } + mrb_gc_mark_value(mrb, t->result); mrb_gc_mark_value(mrb, t->name); t = t->next; From e0d6d39ce98e64c167b0da3f35ed1b07a34a3978 Mon Sep 17 00:00:00 2001 From: HASUMI Hitoshi Date: Thu, 28 May 2026 19:18:17 +0900 Subject: [PATCH 4/4] Write validate flag (status) after setting timeslice `mrb_tick()` observes `status == RUNNING` before touching `timeslice`, so initializing `timeslice` first avoids exposing a partially initialized running state --- mrbgems/mruby-task/src/task.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mrbgems/mruby-task/src/task.c b/mrbgems/mruby-task/src/task.c index fbca3ceec..5aa47c1ef 100644 --- a/mrbgems/mruby-task/src/task.c +++ b/mrbgems/mruby-task/src/task.c @@ -323,8 +323,8 @@ execute_task(mrb_state *mrb, mrb_task *t) uint8_t prev_cci; /* Set task as running */ - t->status = MRB_TASK_STATUS_RUNNING; t->timeslice = MRB_TIMESLICE_TICK_COUNT; + t->status = MRB_TASK_STATUS_RUNNING; /* Switch to task context */ prev_c = mrb->c;