[Service] fix use-after-free on training offloading destroy

[myungjoo]

Addresses item H3 of #690.

The defect

_ml_service_training_offloading_destroy() tore the handle down in this order:

1. g_hash_table_destroy (training_s->node_table) — frees every ml_service_node_info_s
2. ml_pipeline_destroy (training_s->pipeline_h)

and never called ml_pipeline_stop().

Each output node in the received pipeline is registered with
ml_pipeline_sink_register (..., _ml_service_pipeline_sink_cb, node_info, ...),
and that callback dereferences node_info->mls and node_info->name. It also
passes node_info->name into the event data through _ml_info_set_value(),
which stores the pointer without copying it, so a callback that is still running
when the node table goes away reads freed memory.

ml_service_stop() is not mandatory before ml_service_destroy(), so a receiver
that is still PLAYING when the application destroys it can have a buffer reach
the sink after step 1 and run the callback on a freed node_info.

The fix

Release the pipeline before the node table, mirroring
_ml_service_extension_destroy():

• ml_pipeline_stop() takes the pipeline out of PLAYING.
• ml_pipeline_destroy() brings it to GST_STATE_NULL. It blocks on the
  element lock that cb_sink_event() holds for the whole user callback, and
  joins the streaming threads, so no sink callback can be in flight once it
  returns.
• only then is node_table destroyed.

Stopping first also narrows the separate window in which ml_pipeline_destroy()
frees its named nodes while the pipeline is still running.

Tests

destroyWhileRunning_p drives a receiver to PLAYING and destroys it without
stopping — the exact reproduction from the issue. The pipeline is injected the
way the remote sender would send it, but is self-contained (videotestsrc into
tensor_sink) so the test covers the teardown order rather than nntrainer.

The sink callback signals a condition variable on entry and then holds the
streaming thread for 300 ms; the test waits for that signal before destroying,
so the destroy always overlaps the callback rather than usually overlapping it.
After the hold, the callback reads the node name back out of the event data.
With the old order the node table is freed while the callback is still parked,
so that read lands on released memory.

What makes that read fail is allocator behaviour, and it is worth being precise
about which. M_PERTURB is enabled for the duration of the destroy call — and
cleared right after, to keep it off the rest of the suite — but glibc returns
from tcache_put() before free_perturb() runs, and the node name is tcache
sized, so for that chunk M_PERTURB is inert. The clobber the assertion
actually sees is tcache_put() writing its own link fields over the first 16
bytes of the freed chunk, which is reliable on glibc but is still not a property
of the code under test. M_PERTURB stays as a second net for the paths where
the chunk does not go to the tcache. A sanitizer or valgrind job would be the
only airtight net here; that is repository-wide CI work and belongs in its own
issue.

destroyAfterStop_p keeps the documented stop-then-destroy order working now
that destroy stops the pipeline itself, and destroyInvalidParam2_n covers the
guard that rejects a service which is not in training mode.

Deliberately left out

• _training_offloading_send_trained_model() still runs before the stop.
  Moving the stop above it would not make the transfer deterministic:
  ml_pipeline_stop() only pauses, it does not make tensor_trainer finalize
  the model file, so an earlier stop could ship a different model rather than a
  more complete one. That is a behaviour decision for the training offloading
  protocol, not part of this use-after-free.
• g_cond_clear / g_mutex_clear before g_thread_join in the same
  function. Real UB, and tracked separately as item M6 of [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690, which also
  covers the is_received reset and the spurious-wakeup predicate in
  _training_offloading_check_received_data(). Fixing only the teardown half
  here would leave that item ambiguous.

Not verified locally

These tests need nnstreamer, nnstreamer-edge, mlops-agent and nntrainer, which
are not installed on the machine this was written on, so they have not been run
locally — only the GBS CI job with unit_test 1 exercises
unittest_capi_service_training_offloading.

🤖 Generated with Claude Code

[myungjoo]

Note: This review was produced by a separate review agent (a Claude Code sub-agent). I am relaying its findings here verbatim.

Summary

The product-code change is correct, minimal and well-targeted. I verified the claimed mechanism end-to-end in the tree rather than taking the description at face value:

• cb_sink_event() (c/src/ml-api-inference-pipeline.c:282) holds elem->lock (:307) for the entire duration of the user callback.
• ml_pipeline_destroy() (:1173) calls g_hash_table_destroy (p->namednodes) at :1193, whose free func cleanup_node() (:544) takes e->lock at :548.
• Therefore ml_pipeline_destroy() provably blocks until any in-flight sink callback has returned, and after it returns no callback can touch node_info. Moving g_hash_table_destroy (training_s->node_table) after it genuinely closes the use-after-free on node_info->mls / node_info->name that _ml_service_pipeline_sink_cb() (c/src/ml-api-service.c:952) dereferences.

Regression risk looks low:

• The new order is the same shape as _ml_service_extension_destroy() (c/src/ml-api-service-extension.c:716-725), so the two ml-service backends now agree.
• The added ml_pipeline_stop() is effectively a no-op in the non-racy paths: construct_pipeline_internal() already leaves a freshly constructed pipeline in PAUSED, and ml_pipeline_destroy() already pauses a PLAYING pipeline itself (:1200-1208). So it does not introduce a new state transition for the "created but never started" case.
• The sender path is unaffected (node_table is only populated by _training_offloading_conf_parse_pipeline() on the receiver side).
• The diff is 11 lines of product code in one function and touches no other module. Size is proportionate to the topic.
• No public API / ARCHITECTURE change, so no doc update is owed here. ml_service_destroy() in c/include/ml-api-service.h:272 needs no wording change.
• The test-only #include <nnstreamer-edge.h> builds: nns_capi_service_dep re-exports ml_service_deps, which contains nnstreamer_edge_dep (c/src/meson.build:113-149), and the target is gated on support_training_offloading (tests/capi/meson.build:69-77).

Below are the issues I would like addressed before this leaves draft. None of them are in the product-code fix itself; they are all about whether the test can actually keep the bug from coming back.

---

1. (Medium) destroyWhileRunning_p cannot be relied on to turn CI red on the buggy code

This is the item I care most about, because it is exactly the "can CI block a re-regression?" question.

The only thing in the new test that distinguishes the old order from the new order is:

EXPECT_EQ (ml_information_get (event_data, "name", (void **) &node_name), ML_ERROR_NONE);
EXPECT_STREQ (node_name, "training_result");

With the old order, node_name points into memory already released by g_free(). Whether EXPECT_STREQ then fails is allocator-defined behaviour, not a property of the code under test. The PR body is honest about this ("Under valgrind or ASAN the old order is a hard use-after-free"), but the repository does not provide that safety net:

• A case-insensitive grep for asan / valgrind / sanitize across .github/, packaging/, meson.build and the spec file returns zero hits. There is no sanitizer or valgrind job anywhere.
• The Tizen job does not run the tests through meson test either: packaging/machine-learning-api.spec:437 invokes packaging/run_unittests.sh, which executes the binary directly. So meson's automatic random MALLOC_PERTURB_ is not applied.

In practice glibc's tcache will clobber the first 16 bytes of the freed 16-byte "training_result" chunk with its next/key fields, so it will probably fail today on glibc — but "probably, because of tcache internals" is precisely what a regression guard must not depend on. On a different allocator, or after a glibc change, this test silently goes green against the reintroduced bug.

Concrete options, any one of which would fix it:

• Call mallopt (M_PERTURB, 0xAA) in MLServiceTrainingOffloading::SetUpTestSuite(). It takes effect at runtime on glibc and makes free() scrub deterministically. (Trade-off: it applies to the whole binary and may surface other latent UAFs — arguably a feature, but it should be a deliberate decision.)
• Export MALLOC_PERTURB_ from packaging/run_unittests.sh and/or add it to testenv in tests/capi/meson.build.
• Add a sanitizer build job. That would also cover the sibling issues in [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690.

The good news: once the test does fail, gating works — packaging/run_unittests.sh propagates the gtest exit code (exit $?), the %check section aborts the rpmbuild, and gbs_build.yml:17 runs one matrix arch with --define "unit_test 1" while nntrainer_support defaults to 1 (machine-learning-api.spec:20). So the plumbing is fine; only the detection is soft.

2. (Low) The callback/teardown overlap is probabilistic rather than enforced

_start_receiver_pipeline() polls every 10 ms until received > 0, and the callback parks for 300 ms, so today the main thread almost certainly reaches the node_table free while the callback is still parked. But nothing guarantees it. On a loaded CI machine a >300 ms scheduling gap makes the test pass even with the bug — a silent false negative, and one that gets worse as the suite grows.

Suggestion: have _hold_new_data_cb signal a GCond on entry and have the main thread wait on it before calling ml_service_destroy(). That converts "usually overlaps" into "always overlaps" and also lets you shorten the 300 ms hold, which currently costs wall-clock time in two tests.

3. (Low) ml_pipeline_stop()'s return value is dropped silently

ml_pipeline_stop (training_s->pipeline_h);

Every other call in this function at least logs on failure (see the ml_pipeline_destroy() call three lines below). ml_pipeline_stop() can return ML_ERROR_STREAMS_PIPE on GST_STATE_CHANGE_FAILURE and ML_ERROR_NOT_SUPPORTED from check_feature_state() on Tizen, and a failure here means the subsequent ml_pipeline_destroy() is doing the racy pause itself. A one-line if (ML_ERROR_NONE != ...) _ml_error_report (...) would make that diagnosable. (_ml_service_extension_destroy() has the same omission, so this is consistency-vs-hygiene; your call.)

4. (Low) The fix stops short of a sibling ordering problem in the same function

Two things in _ml_service_training_offloading_destroy() still run before the pipeline is stopped:

• _training_offloading_send_trained_model (mls) at c/src/ml-api-service-training-offloading.c:898 reads trained_model_path off disk and ships it to the remote sender while the training pipeline may still be writing that file. Given this PR is specifically about "stop the pipeline before touching what it feeds", moving the stop above line 896 would make the transferred model deterministic. I understand that is a behaviour change beyond the UAF fix and may belong in its own PR — but it is worth an explicit decision rather than being left implicit.
• g_cond_clear (&training_s->received_cond) / g_mutex_clear (&training_s->received_lock) at :901-902 run before g_thread_join (training_s->received_thread) at :904-906. If _check_received_data_thread() is still alive — which is exactly the "destroy while running" scenario this PR targets — it will g_mutex_lock / g_cond_signal an already-cleared primitive. That is UB of the same class as the bug being fixed, in the same function, and the new tests happen to dodge it only because the checker thread has already exited by then. Either fold it in or file it explicitly under [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690 so it is not lost.

5. (Info) Minor test-side nits

• destroyInvalidParam2_n leaks the ml_service_s: _ml_service_offloading_release_internal() frees mls->priv only, so the handle from _ml_service_create_internal() (plus its ml_option, GCond, GMutex) is never released. This matches the pre-existing create_p / createInvalidParam1_n pattern, so it is not a new sin, but the PR adds one more instance. It also duplicates ~90% of create_p's JSON-loading preamble — a small helper would shrink both.
• destroyInvalidParam2_n exercises a guard that is unreachable from production, since _ml_service_offloading_release_internal() only calls _ml_service_training_offloading_destroy() when the mode is TRAINING. Fine as defensive coverage, just noting it is not the case the issue is about.
• Nit on the PR description, not the code: "The application also holds a dangling name for as long as it holds the event data" overstates it — _ml_service_invoke_event_new_data() destroys the ml_information_h immediately after the callback returns, and the API already documents event_data as valid only inside the callback. The real exposure is the in-flight callback, which the rest of the description describes correctly.

6. (Info) Not run anywhere yet

The PR notes the tests have not been executed locally. Given that both new tests are timing-sensitive and one of them asserts on freed memory, please make sure the GBS job with unit_test 1 has actually gone green — and ideally that destroyWhileRunning_p has been observed to fail on the pre-fix commit — before the draft / DO NOT MERGE state is lifted. A regression test that has never been seen to fail is not yet a regression test.

---

Verdict: the fix itself is sound and I would take it as-is. The blocking concern is item 1 — as written, the accompanying test does not reliably convert a re-regression into a red CI, which is the main thing this PR is supposed to buy us beyond the one-line reorder.

[myungjoo]

Thanks — going through the review point by point.

1. (Medium) The test could not be relied on to turn CI red — fixed

Agreed, and this was the right thing to lead with. mallopt (M_PERTURB, 0xAA) is
now set immediately before ml_service_destroy() in destroyWhileRunning_p and
cleared immediately after, so glibc scrubs what it releases during exactly the
window where the broken order frees the node table. The stale name then fails
EXPECT_STREQ because of the code under test, not because of tcache internals.

Scoping it to that one call rather than SetUpTestSuite() was deliberate: the
suite-wide version would change allocator behaviour for trainingOffloading_p
and the whole nntrainer path too, and I would rather not have this PR go red for
an unrelated latent defect. The knob is guarded with #ifdef __GLIBC__.

I did not take the MALLOC_PERTURB_ / sanitizer-job options here — both are
build-infrastructure changes that affect every test in the repository, which is
more than this PR should decide on its own. A sanitizer job would be worth its
own issue; it would also cover the rest of #690.

2. (Low) Probabilistic overlap — fixed

The 10 ms poll is gone. _hold_new_data_cb now signals a GCond on entry and
_start_receiver_pipeline() waits on it (10 s deadline) before returning, so the
destroy provably starts while the callback is held. The 300 ms hold now only has
to outlast the teardown itself rather than also covering the detection latency.

3. (Low) ml_pipeline_stop() return dropped — fixed

Now logged, in the same shape as the ml_pipeline_destroy() call below it. I
left _ml_service_extension_destroy() alone; it is not in this PR's path.

4. (Low) Sibling ordering problems — one declined, one deferred

• _training_offloading_send_trained_model() before the stop: declining, and
  I think the premise does not hold. ml_pipeline_stop() only pauses the
  pipeline; it does not make tensor_trainer finalize the model file, which
  nntrainer writes on epoch/EOS boundaries. So moving the stop above line 896
  would not make the transferred model deterministic — it could just as easily
  ship a different one. Which model a receiver is supposed to reply with is a
  protocol decision, not part of this use-after-free. Recorded in the PR
  description so the decision is explicit rather than implicit.

• g_cond_clear / g_mutex_clear before g_thread_join: agreed it is UB,
  and it is already filed — it is item M6 of [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690. M6 also covers the
  is_received reset and the spurious-wakeup predicate in
  _training_offloading_check_received_data(); fixing only the teardown half
  here would leave that checklist item half-done and untested. It is called out
  in the PR description so it is not lost.

5. (Info) Test nits — fixed

• destroyInvalidParam2_n now releases the handle with
  _ml_service_destroy_internal(), which drops the ml_service_s and its
  ml_option as well as the offloading private data. Good catch — the
  pre-existing tests do leak this.
• On the JSON preamble duplication: agreed it is repetitive, but factoring it
  out means touching create_p, createInvalidParam1_n and
  createInvalidParam2_n, which are not otherwise in this diff. Leaving it.
• The overstatement about a dangling name outliving the callback is removed
  from the PR description; you are right that _ml_service_invoke_event_new_data()
  destroys the information handle as soon as the callback returns.

6. (Info) Never run

Still true and still stated in the PR description — the toolchain for these
tests is not available locally. The draft and DO NOT MERGE state stays until
the GBS unit_test 1 job is green.

[myungjoo]

Note: This is a re-review produced by a separate review agent (a Claude Code sub-agent). I am relaying its findings here verbatim.

Verdict

No blocking issues remain. The product-code fix is correct and I would merge it once the GBS unit_test 1 job is green (item 6 of the previous round is the only thing still outstanding, and it is a process gate, not a code defect).

I re-verified the two new commits (88b4341, b0b223a) against the tree rather than against the response comment. Items 2, 3, 4 and 5 are genuinely resolved or validly declined. Item 1 needs a factual correction to the commit message / code comment — the mallopt does not do what it says it does — but the test still detects the regression deterministically on glibc for a different reason, so this is a doc-accuracy issue, not a blocker.

---

(a) mallopt (M_PERTURB, 0xAA) — the stated mechanism is wrong (Low, non-blocking)

The claim under review is "glibc scrubs what it releases during exactly the window where the broken order frees the node table". I checked this against glibc itself rather than assuming:

• glibc 2.35 _int_free (malloc/malloc.c): the #if USE_TCACHE block ends with tcache_put (p, tc_idx); return; at line 4481. The two free_perturb (chunk2mem(p), size - CHUNK_HDR_SZ) calls are at lines 4525 (fastbin path) and 4598 (consolidate path) — after that early return.
• glibc 2.39: same shape — tcache_put/return at 4549, free_perturb at 4593 and 4686.
• glibc 2.41: refactored, but the ordering is identical — tcache_free() (which calls tcache_put) runs inside __libc_free before _int_free, and free_perturb lives only in _int_free_chunk / _int_free_merge_chunk.

node_info->name is g_strdup ("training_result") → a 16-byte request → a 32-byte chunk → tcache bin. ml_service_node_info_s is ~32 bytes → also tcache. So for both allocations the free returns via tcache and free_perturb() is never reached. mallopt (M_PERTURB, 0xAA) is inert here. (For the same reason the stated worry about it perturbing other threads' allocations during that window is also moot — __libc_malloc's tcache hit path skips alloc_perturb symmetrically.)

What actually clobbers the string is tcache_put() writing e->next (mangled by PROTECT_PTR) and e->key over the first 16 bytes of the freed chunk — which is exactly the "glibc tcache internals" dependency the previous round's item 1 objected to. So in substance nothing changed; only the justification did, and the new justification is incorrect.

To be fair to the change, the practical outcome is still deterministic on glibc, and I want to state that clearly so this is not read as worse than it is:

• if the tcache bin has room → tcache_put overwrites bytes 0..15;
• if the bin is full, or tcache is disabled via glibc.malloc.tcache_count=0 → the fastbin / consolidate path runs, and there M_PERTURB does fire.

So under glibc the first 16 bytes of that chunk are overwritten on every path, and EXPECT_STREQ (node_name, "training_result") will fail on the buggy order. The #ifdef __GLIBC__ guard plus the project's targets (Tizen / Ubuntu) make that acceptable.

Requested change (small): fix the wording in 88b4341's commit message, the PR description, and the in-test comment so it says what is actually true — the check relies on glibc overwriting a freed small chunk (tcache metadata, or M_PERTURB on the non-tcache paths), not on M_PERTURB alone. Otherwise the next person to touch this test will reason from a false premise. If you want a real, allocator-independent guarantee later, the two options are still a sanitizer job or GLIBC_TUNABLES=glibc.malloc.tcache_count=0 in testenv / run_unittests.sh; both are the repo-wide infra changes you reasonably declined for this PR.

(b) The GCond handshake — resolved (verified)

_hold_new_data_cb sets entered and broadcasts under hold->lock before the g_usleep (SINK_CB_HOLD_TIME), and _start_receiver_pipeline() only returns after observing entered (loop guards spurious wakeups, 10 s deadline, ASSERT_TRUE (entered) on timeout). So ml_service_destroy() provably starts while the callback is parked. That is a real improvement over the 10 ms poll.

Residual (Info, no action needed): the detection still needs the 300 ms hold to outlast the main thread's path from the broadcast down to g_hash_table_destroy (node_table). I walked that path — _training_offloading_send_trained_model() bails on a failing g_file_get_contents of a non-existent <tmpdir>/trained-model.bin, the checker thread has already returned (it exits immediately after signalling, since receiver_pipe_json_str is set before _start()), so g_thread_join is instant, then two hash-table destroys. Microseconds. A >300 ms deschedule there would give a false negative (green on buggy code), never a false positive. Acceptable.

Worth noting that the fix is stronger than the PR describes: because gst_base_sink_chain holds the preroll lock across render, the new ml_pipeline_stop() itself blocks until the in-flight sink callback returns, before ml_pipeline_destroy() is even entered.

(c) _ml_service_destroy_internal() in destroyInvalidParam2_n — resolved (verified correct)

mls->type is ML_SERVICE_TYPE_OFFLOADING, so _ml_service_destroy_internal() (c/src/ml-api-service.c:113) dispatches to _ml_service_offloading_release_internal() — the same call the old code made — and additionally destroys mls->information, clears mls->cond / mls->lock and frees the struct. No double-free risk: the preceding _ml_service_training_offloading_destroy() bails out in _training_offloading_get_priv() because the mode is NONE, so it frees nothing, and _ml_service_offloading_release_internal() skips the training branch for the same reason. It returns ML_ERROR_NONE unconditionally, so the EXPECT_EQ holds.

Item 4 — both dispositions are sound

• _training_offloading_send_trained_model() before the stop — decline accepted. I checked ml_pipeline_stop() (c/src/ml-api-inference-pipeline.c:1337): it is gst_element_set_state (p->element, GST_STATE_PAUSED) and nothing else. It does not EOS the pipeline and does not make tensor_trainer finalize its output. The premise of my earlier suggestion does not hold; moving the stop up would change which model is shipped without making it deterministic. Correctly out of scope.
• g_cond_clear / g_mutex_clear before g_thread_join — deferral accepted. Still present at :901-906. I confirmed the new tests cannot trip it: _check_received_data_thread() returns as soon as receiver_pipe_json_str != NULL, which the test sets before _start(), and it does its final g_mutex_unlock before returning, so the clear on the main thread is ordered after it. Fine to leave to M6 of [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690, which is explicitly referenced in the PR body.

Regression / scope checks

• Product diff is 11 lines in one function; nothing else in c/src is touched. Proportionate to the topic.
• The added ml_pipeline_stop() introduces no new state transition on the non-racy paths: construct_pipeline_internal() already leaves a freshly constructed pipeline in PAUSED by calling ml_pipeline_stop() itself (:1093), and ml_pipeline_destroy() already pauses a PLAYING pipeline (:1200-1208). PAUSED → PAUSED yields SUCCESS / NO_PREROLL, never FAILURE, so b0b223a's new log will not fire spuriously in create_p / trainingOffloading_p.
• No new deadlock class. ml_pipeline_stop() blocks holding p->lock while the sink callback is parked; cb_sink_event() takes only elem->lock (and mls->lock, released before the user callback is invoked), and cb_bus_sync_message() takes no pipeline lock at all. ml_pipeline_destroy() already had exactly this blocking shape via cleanup_node().
• No new double-free from the reorder: _training_offloading_node_info_free() releases only name and the struct, never node_info->handle, which ml_pipeline_destroy() now frees first via cleanup_node() / free_element_handle().
• mallopt does not leak into other tests: it is set and cleared around the single ml_service_destroy() call, and (per (a)) is inert for tcache-sized chunks anyway.
• Sender path unaffected — node_table is only populated by _training_offloading_conf_parse_pipeline() on the receiver side.

Static compilability check (no local build available)

• _ml_service_destroy_internal is declared in c/src/ml-api-service-private.h:124, which the test includes; _ml_service_create_internal at :119. _ml_service_offloading_create (ml_service_h, JsonObject *) at ml-api-service-offloading.h:60 — passing ml_service_s * into the void * handle is fine in C++.
• _ml_service_training_offloading_process_received_data (ml_service_s *, void *, const gchar *, const gchar *, int) matches the call site, and nns_edge_data_h is void *.
• <nnstreamer-edge.h> resolves: nns_capi_service_dep re-exports ml_service_deps, which includes nnstreamer_edge_dep (c/src/meson.build:113-149), and the target is gated on support_training_offloading (tests/capi/meson.build:69-77).
• <malloc.h> is included under #ifdef __GLIBC__, and __GLIBC__ is already defined at that point because <gtest/gtest.h> and <glib/gstdio.h> precede it. g_rmdir / g_remove now have their declaring header explicitly.
• R"JSON(...)JSON" is fine under cpp_std=c++14. The build is werror=true, warning_level=1; I found no unused-variable/parameter, missing-field-initializer or sign-compare candidate in the new code. ASSERT_* is used only in void-returning functions; the streaming-thread callback uses EXPECT_* only, which is the correct choice.

CI gating

Unchanged and adequate: packaging/run_unittests.sh propagates the gtest exit code, %check aborts the rpmbuild, and gbs_build.yml runs the x86_64 matrix entry with --define "unit_test 1" while nntrainer_support defaults to 1. The two new tests add well under a second of wall clock each.

Docs / ARCHITECTURE

No public API or architecture change. ml_service_destroy() (c/include/ml-api-service.h) already documents ML_ERROR_STREAMS_PIPE as "Failed to stop the process", so destroy stopping the pipeline itself is consistent with the existing contract. Nothing owed in this PR.

---

Summary: ship it once CI is green. The only follow-up I would ask for is the one-paragraph correction described in (a), since the commit message and the in-test comment currently assert something about glibc that is not true.

[myungjoo]

You are right about M_PERTURB, and the correction is worth making explicitly:
_int_free() takes the tcache branch and returns before it ever reaches
free_perturb(), and both node_info->name (16 bytes) and
ml_service_node_info_s are tcache sized. So the knob is inert for exactly the
two frees the assertion depends on, and my commit message claimed the opposite.

Fixed by rewording rather than by adding machinery, since there isn't machinery
that would honestly help here:

• The in-test comment now says what the detection actually rests on —
  tcache_put() overwriting the first 16 bytes of the chunk with its link
  fields — and that a sanitizer build is the only airtight net.
• The commit message on [Test] make the training offloading teardown test deterministic says the same, and no longer claims M_PERTURB is the
  mechanism. Force-pushed, so the branch is 249ed38 ded70e8 063b4b1 fb80b6d;
  only that one message and one comment block changed, no code.
• The PR description carries the same correction.

I kept M_PERTURB rather than dropping it. It is dead for the tcache path but
does fire when the chunk does not go there — a full tcache bin for that size, or
a larger allocation — so it is a cheap second net rather than a claim. It stays
scoped to the single ml_service_destroy() call.

Agreed that a sanitizer or valgrind job is the real answer, and agreed it does
not belong in this PR: it is repository-wide CI work that would also cover the
rest of #690. Worth its own issue.

Thanks also for the gst_base_sink preroll-lock observation — the new
ml_pipeline_stop() blocking on the in-flight callback is a stronger guarantee
than I had claimed, and it is good to have that on the record.

No functional change since your review; the branch is otherwise as you reviewed
it. Leaving the draft and DO NOT MERGE state until the GBS unit_test 1 job
reports green.

[myungjoo]

CI is green on fb80b6d, and the three new tests did run rather than being
compiled out — from the Tizen GBS x86_64 unit_test 1 job:

[ RUN      ] MLServiceTrainingOffloading.destroyWhileRunning_p
[       OK ] MLServiceTrainingOffloading.destroyWhileRunning_p (416 ms)
[ RUN      ] MLServiceTrainingOffloading.destroyAfterStop_p
[       OK ] MLServiceTrainingOffloading.destroyAfterStop_p (416 ms)
[ RUN      ] MLServiceTrainingOffloading.destroyInvalidParam2_n
[       OK ] MLServiceTrainingOffloading.destroyInvalidParam2_n (12 ms)

Everything else passes too: static checks, spell check, DCO, the four Android
builds, and the armv7l/aarch64 GBS builds.

Taking this out of draft and dropping DO NOT MERGE.

One caveat stays open, and it is the review's item 6: destroyWhileRunning_p
has been seen to pass on the fixed tree, but has not been seen to fail on the
pre-fix tree. The GBS workflow only triggers on pull_request, so demonstrating
that needs a throwaway PR carrying the tests without the fix. Happy to run that
as a negative control if it is wanted before merge.

[myungjoo-bot]

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR moves g_hash_table_destroy (training_s->node_table) after the pipeline teardown and adds ml_pipeline_stop() before ml_pipeline_destroy() in _ml_service_training_offloading_destroy(), plus three tests. The defect was confirmed on upstream/main: at c/src/ml-api-service-training-offloading.c:914-925 the node table (value free func _training_offloading_node_info_free, which frees name and the struct) is destroyed before ml_pipeline_destroy, no stop is issued, and _ml_service_pipeline_sink_cb (c/src/ml-api-service.c:954-966) dereferences node_info->mls / node_info->name and passes name uncopied through _ml_information_set -> _ml_info_set_value (ml-api-common.c:1502-1528). The fix is sound: ml_pipeline_stop sets PAUSED, which in basesink takes the preroll lock held across render and so blocks on an in-flight callback; ml_pipeline_destroy then destroys namednodes, whose cleanup_node (ml-api-inference-pipeline.c:544) takes e->lock — the same lock cb_sink_event (:307-425) holds around the user callback — and sets the pipeline to NULL, joining the streaming threads. Even on the early-error returns of ml_pipeline_destroy (:1205, :1227) namednodes is already gone, so no callback can reach node_info afterwards. The GBS x86_64 unit_test 1 log (run 34106153859) shows destroyWhileRunning_p OK (416 ms), destroyAfterStop_p OK (416 ms), destroyInvalidParam2_n OK (12 ms), 11/11 in the suite; the 416 ms destroy duration is itself evidence that the stop blocked on the parked callback. merge-tree is clean, all 10 checks pass, all four commits are DCO-signed and ordered fix-then-tests.

Also verified: no deadlock (_ml_service_destroy_internal releases mls->lock before the offloading release, so the callback's _ml_service_get_event_cb_info lock cannot contend with the blocking stop); no double free (_training_offloading_node_info_free never touches node_info->handle, which cleanup_node frees); _training_offloading_send_trained_model and the transfer_data_table destruction still precede the stop and do not touch node_table / pipeline; the M6 g_cond_clear / g_mutex_clear-before-join at :901-906 is pre-existing and byte-identical; the order now matches _ml_service_extension_destroy (ml-api-service-extension.c:716-725) except for the added stop-failure log; c/include/ml-api-service.h:262-272 does not require stop-before-destroy and already lists ML_ERROR_STREAMS_PIPE. Test gating is correct: built only under support_training_offloading (tests/capi/meson.build:69-77), run only under nntrainer_support in the spec, never compiled by the Android lanes, and <malloc.h> / mallopt are under #ifdef __GLIBC__. The injected videotestsrc ... tensor_sink pipeline does not depend on nntrainer; the GCond handshake makes the overlap deterministic; the final commit message and in-test comment correctly attribute the observed clobber to tcache_put() metadata rather than M_PERTURB. Approving; the items below are non-blocking.

1. [Low] c/src/ml-api-service-training-offloading.c:916-918: the ml_pipeline_stop failure is logged but not folded into ret, so a failed stop followed by a successful destroy returns ML_ERROR_NONE. Acceptable and consistent with the extension backend; if you want it surfaced, set ret on the stop failure only when the subsequent destroy succeeds.
2. [Low, pre-existing, note only] c/src/ml-api-inference-pipeline.c:544-606 cleanup_node frees e after unlocking e->lock; a streaming thread already inside cb_sink_event and blocked on that lock would touch freed memory (the @todo CRITICAL comment). Stopping first shrinks that window, as the commit message says, but does not close it; worth its own item in #690 if not already there.
3. [Low] tests/capi/unittest_capi_service_training_offloading.cc:467-477: mallopt (M_PERTURB, 0xAA) is process-wide while set (also affects the nns-edge thread) and is inert for the two tcache-sized frees the assertion relies on. Harmless as scoped; it could be dropped without loss of detection. No change required.

No back-door or suspicious behavior found: the product diff is a pure reordering plus one logged call inside one function; the test diff adds only local scaffolding with no network endpoints or file writes outside g_dir_make_tmp.

[myungjoo]

Thanks for the review. Dispositions on the three items — no code change on any of
them, so the branch stays at fb80b6d and the green run stands.

1. Folding a failed ml_pipeline_stop() into ret — declining, on merit

I do not think surfacing it is right here. ret is what ml_service_destroy()
eventually returns, and if the stop fails but the destroy succeeds the handle has
in fact been fully released: the pipeline is at NULL, the node table is gone, the
struct is freed. Returning ML_ERROR_STREAMS_PIPE from that would tell the
application its ml_service_destroy() failed when nothing leaked and there is
nothing left for it to retry — and there is no retry to offer, since the handle
is already invalid.

It would also make destroyWhileRunning_p and destroyAfterStop_p flaky:
EXPECT_EQ (ML_ERROR_NONE, status) would start depending on a state change
succeeding, which is exactly the timing-sensitive part.

So the log stays a log. It matches the extension backend, as you note.

2. cleanup_node freeing e after unlocking e->lock — already filed

It is item M13 of #690, verbatim: "cleanup_node unlocks e->lock, clears it
and g_free(e); a streaming thread waiting on that lock in cb_sink_event
resumes on freed memory", with the suggested fix of bringing the pipeline to NULL
before destroying namednodes, or refcounting ml_pipeline_element. So it is not
lost, and it is correctly out of scope here — it is a defect in the pipeline layer
that affects every ml_pipeline_destroy() caller, not just this backend.

Agreed on the characterisation: the added stop shrinks that window, it does not
close it.

3. mallopt (M_PERTURB, 0xAA) — keeping it, with the caveat already recorded

You are right that it is inert for the two tcache-sized frees the assertion turns
on; that is what the in-test comment and the commit message now say, after the
previous review round caught me claiming otherwise. It is kept as a second net for
the case where the chunk does not reach the tcache — a full bin for that size
class — where free_perturb() does run and produces the same detection.

Process-wide-while-set is a fair point. It is bounded to the single
ml_service_destroy() call and cleared immediately after, and the only other
thread live in that window is the nns-edge receive thread, which is idle here
(no peer is connected in this test). I would rather keep the narrow extra
coverage than drop it for tidiness, but it is genuinely marginal either way.

Standing agreement from both review rounds: a sanitizer or valgrind job is the
only airtight net for this class of defect, and it belongs in its own issue since
it is repository-wide CI work.