Client task_list never unlinks FINISHED tasks (libgearman/run.cc:418)

[p-alik]

Summary

gearman_client_st::task_list never unlinks a task when it transitions to GEARMAN_TASK_STATE_FINISHED. The only removal path is the auto-free branch at the bottom of _client_run_task():

// libgearman/run.cc:423-426 (current master)
if (task->client->options.free_tasks and task->type == GEARMAN_TASK_KIND_ADD_TASK)
{
  gearman_task_free(task->shell());
}

This only fires when GEARMAN_CLIENT_FREE_TASKS is set and the task is of kind GEARMAN_TASK_KIND_ADD_TASK. For any client that doesn't set that option, or that uses other task kinds, finished tasks accumulate on task_list for the lifetime of the gearman_client_st — they're only ever removed by an explicit gearman_task_free() call or by freeing the whole client. Long-running clients that submit many one-off tasks can see task_list (and thus the per-packet scan cost and memory footprint) grow unbounded.

This is pre-existing behavior, not something introduced by #435 — that PR left run.cc untouched. It surfaced during review of #435 because switching the client task list from LIFO (prepend-to-head) to FIFO (append-to-tail) moved already-finished-but-unlinked tasks from the tail (scanned last) to the head (scanned first) of the packet-matching loop, which is what turned this latent issue into an actual hang (fixed in #435 via an is_active() guard in the scan loop). The guard fixes the symptom; this issue is about the underlying cause.

Why this needs care, not a one-line fix

The three packet-matching loops in _client_run_tasks() (libgearman/client.cc:1453, :1489, :1542) walk task_list with:

for (client->task= client->task_list; client->task;
     client->task= client->task->impl()->next)

...calling _client_run_task() (which reaches the FINISHED transition at run.cc:418) from inside the loop body, before the increment reads ->next. Unlinking naively at that transition breaks these loops:

• If the unlink also clears the task's own next/prev, finishing a task while it's the loop's current node makes the increment read the just-cleared next (NULL), silently ending the scan early for that pass.
• If it leaves next/prev untouched, a single removal is fine, but two adjacent tasks finishing before either is freed via gearman_task_free() leaves stale pointers, so the second free can re-splice an already-removed node back into the list, corrupting it.

Proposed fix

Restructure the three scan loops in _client_run_tasks() to snapshot task->impl()->next before running the task, then perform the unlink (and task_list_tail update, once #435's FIFO change lands) at the GEARMAN_TASK_STATE_FINISHED transition in run.cc, mirroring what gearman_task_free() already does for head/tail bookkeeping. Alternatively/additionally, consider extending the free_tasks auto-free branch to cover all task kinds, not just GEARMAN_TASK_KIND_ADD_TASK.

Context

Raised during review of #435 (LIFO→FIFO client task list change) by @p-alik, agreed as a follow-up by @esabol:
#435 (comment) and follow-up discussion.

[p-alik]

Before implementing, I traced through the actual call sites and found something that changes the scope of the fix.

The three scan loops aren't all equally at risk. Of the loops walking task_list in _client_run_tasks() (client.cc:1453, :1489, :1542), only the first two (:1453, :1489) actually call _client_run_task() from inside the loop body. Neither can reach GEARMAN_TASK_STATE_FINISHED within that same call — every path through NEW/SUBMIT/WORKLOAD returns at the WORK state (see run.cc:181) rather than falling through to the completion states. The third loop (:1542) doesn't call _client_run_task() at all, and the main packet-matching loop calls it exactly once, outside any for-loop, with a fresh re-scan from task_list head on the next outer iteration. So the "increment reads a just-cleared ->next" corruption scenario doesn't actually apply to these loops as originally described.

This holds true regardless of whether #435 has merged yet: #435's only change to _client_run_tasks() is adding an is_active() guard inside the packet-matching loop, which already didn't call _client_run_task() from inside its body either before or after that PR — the actual call happens once, afterward, outside any for-loop. Loops :1453 and :1489 (the two that do call it in-loop) aren't touched by #435, and the FIFO/tail-pointer change only affects insertion/removal order, not the state-machine logic that determines whether a task can reach FINISHED within one call. So this analysis doesn't need revisiting once #435 lands.

The real corruption risk is different: it's gearman_task_free()'s prev/next splicing being non-idempotent on a node that's already been unlinked-but-not-freed. If task B is unlinked at FINISHED without clearing its own prev/next, and its neighbor C is unlinked later, then an eventual explicit gearman_task_free(B) re-splices using B's stale prev/next, potentially re-linking a node (C) that's already been removed (or freed) — a real use-after-free risk, just not the one originally described.

Bigger issue: unconditional unlink-at-FINISHED breaks gearman_execute_by_partition. Its reduce step (job.cc complete(), lines 138-166) calls gearman_client_run_tasks(client) to drive all partition sub-tasks to completion, then reads client->impl()->task_list afterward to check error codes and feed aggregator_fn. If sub-tasks are unlinked from task_list the moment they hit FINISHED — which happens during run_tasks() — task_list would be empty by the time complete() reads it, and the aggregator would silently never run. This isn't a rare edge case: I confirmed those sub-tasks default to GEARMAN_TASK_KIND_ADD_TASK, the exact same kind already covered by the existing (opt-in) free_tasks auto-free branch in run.cc:424.

Proposed scoping, for discussion:

1. Safe option — extend the existing GEARMAN_CLIENT_FREE_TASKS auto-free branch to cover all task kinds, not just ADD_TASK. Stays opt-in, no default-behavior change, no risk to execute_by_partition callers (they simply wouldn't set the flag). Doesn't address growth for clients that never set free_tasks, but that's already their choice today.
2. Default-on option — unlink at FINISHED for everyone by default, but explicitly exclude whichever tasks feed an aggregator so execute_by_partition keeps working. Fixes the growth problem broadly, but is a bigger behavior change to make this close to a 2.0.0 release, and needs a reliable way to identify "this task feeds an aggregator" beyond just its GEARMAN_TASK_KIND.

Leaning toward (1) unless there's an appetite for (2) despite the release timing. Thoughts?

[esabol]

Is this Claude's idea? The more I've thought about this, the more I'm actually not sure this is a good idea or if it's even a problem. The changes sound pretty extensive, and gearmand has always worked this way without anyone thinking it was a problem.

[p-alik]

Yes — I used Claude to help draft the analysis in my comment above, but every claim in it is backed by specific line references I verified against current master, not just generated prose. Concretely:

• run.cc:415-427: unlink only happens via the opt-in free_tasks + ADD_TASK branch, confirmed by reading the code.
• client.cc:1453/:1489/:1542/:1672: traced the Duff's-device-style state machine line by line to confirm which loops call _client_run_task() in-body vs. outside the loop.
• job.cc:138-166 + task.hpp:95: confirmed partition sub-tasks default to GEARMAN_TASK_KIND_ADD_TASK, and that complete() reads task_list after run_tasks() returns.

So I'd push back gently on "is this even a problem" — the growth behavior is real and reproducible from the code as written, not speculative. That said, I think your skepticism is fair on a different axis: this only bites long-lived gearman_client_st objects that submit many one-off tasks without ever setting free_tasks or calling gearman_task_free() themselves — gearman_client_free() already reclaims everything, so most callers never see it. Given that narrower blast radius and the 2.0.0 timing, I'm fine deferring this or scoping it down to option (1) (opt-in-only) rather than doing a default-behavior change now. Happy to close this as low-priority / opt-in-only if that's the consensus.

[esabol]

@p-alik wrote:

That said, I think your skepticism is fair on a different axis: this only bites long-lived gearman_client_st objects that submit many one-off tasks without ever setting free_tasks or calling gearman_task_free() themselves — gearman_client_free() already reclaims everything, so most callers never see it. Given that narrower blast radius and the 2.0.0 timing, I'm fine deferring this or scoping it down to option (1) (opt-in-only) rather than doing a default-behavior change now. Happy to close this as low-priority / opt-in-only if that's the consensus.

OK, this all makes a lot more sense to me now as to why nobody has ever reported a problem with the current state. These days, I suspect most programmers using Gearman are submitting tasks/jobs from Python or PHP (or Perl, except your Gearman::Client doesn't use libgearman, of course), and I suspect most client objects in that context are not long-lived.

I think scoping it down to option (1) for the 2.0.0 release sounds very reasonable to me, but it's up to you if you want to put in the work on a more comprehensive fix.

[p-alik]

Dug into option (1) more and it's not the config-flag change it sounded like. Dropping the ADD_TASK-only guard at run.cc:424 so free_tasks also covers DO/EXECUTE kinds introduces a use-after-free:

• do.cc:client_do() reads do_task->impl()->error_code()/result after run_block_tasks() returns, then explicitly frees it — auto-freeing DO-kind tasks mid-run would null-deref there.
• gearman_execute()/gearman_execute_by_partition() return the task pointer to the caller after running it — auto-freeing EXECUTE-kind tasks would hand back a dangling pointer.

So doing this safely means auditing every DO/EXECUTE call site plus new regression tests — more like half a day, not five minutes.

Given the narrow blast radius we already agreed on (long-lived clients doing one-off ADD_TASKs without free_tasks), I'd lean toward closing this as low-priority/wontfix for 2.0.0. Happy to do the audit-and-fix version if you'd rather keep it open.

[esabol]

I kind of wondered if client task objects were meant to be referenced after completion. I suggest checking with the usage in the PHP extension to make sure any changes along this line don't trigger any use-after-frees there as well.

Let's keep the issue open unless you don't think it should be changed? But I take it you're in favor of at least deferring until after 2.0.0 is released?

[p-alik]

Checked the PHP extension (php/pecl-networking-gearman) — it's not just a theoretical downstream risk, it makes the use-after-free concrete and universal rather than edge-case.

php_gearman_client.c:38 sets GEARMAN_CLIENT_FREE_TASKS unconditionally in GearmanClient::__construct() — every PHP client has free_tasks on, not just ones that opt in. It also registers a context-free callback (gearman_client_set_task_context_free_fn, :41) that libgearman only invokes from inside gearman_task_free() (task.cc:93-95); PHP relies on that firing exactly once, deterministically, to drop its internal refcount on the $task object (zend_hash_index_del in _php_task_free, php_gearman_task.c:73).

• Async addTask() (ADD_TASK-kind) — already covered by today's opt-in free_tasks+ADD_TASK branch at run.cc:424, so this path is already exercised safely in production.
• Synchronous doNormal()/doHigh()/doLow() (DO-kind) — this is where extending free_tasks to DO-kind (option 1) breaks things. These route through client_do() in do.cc, which (1) calls gearman_client_run_block_tasks(), (2) then reads do_task->impl()->error_code()/result, (3) then explicitly calls gearman_task_free(do_task) (do.cc:130, same pattern at client.cc:139/204/249/264/912/940/1009). If free_tasks also auto-frees DO-kind tasks at FINISHED, the task gets freed mid-step-(1), before do.cc's own reads in step (2) — a UAF read — and step (3)'s explicit free becomes a double-free. Because GEARMAN_CLIENT_FREE_TASKS is unconditional in the PHP constructor, this wouldn't be a rare long-lived-client scenario — it would hit every PHP script that calls doNormal/doHigh/doLow.

One narrowing data point: PHP never touches the newer gearman_execute()/gearman_execute_by_partition() EXECUTE-kind API, so that half of the concern doesn't apply to this particular consumer — just the DO-kind path, which lines up with what was already flagged from do.cc directly.

Net: this reinforces sticking with option (1) scoped narrowly (i.e. not extending the guard to DO/EXECUTE kinds), or deferring entirely past 2.0.0. I'm fine leaving this open and low-priority.

[esabol]

Thank you for the analysis of how the PHP extension uses libgearman. That makes the path forward much clearer.