[C-Api] validate the flexible tensor header in the sink callback

[myungjoo]

Addresses item M9 of #690.

The defect

cb_sink_event() (c/src/ml-api-inference-pipeline.c) handled a flexible tensor like this:

gst_tensor_meta_info_parse_header (&meta, map[i].data);
hsize = gst_tensor_meta_info_get_header_size (&meta);
...
_data->tensors[i].data = map[i].data + hsize;
_data->tensors[i].size = map[i].size - hsize;

map[i] is the mapped GstMemory that arrived at the sink, so map[i].size is whatever the upstream element produced. Nothing in those four lines is checked.

The parser reads before it validates. gst_tensor_meta_info_parse_header() fills the whole GstTensorMetaInfo — 88 bytes, val[0] through val[21] — out of the pointer it is given, and only then calls gst_tensor_meta_info_validate(). A memory shorter than the struct is read past its end.

The return value is dropped. hsize is used whether or not the meta was accepted.

The subtraction wraps. map[i].size - hsize is gsize arithmetic. When the memory is shorter than the header it declares, the callback receives a pointer past the end of the mapped region together with a size near 2^64.

Measured on main (8952123), a 96-byte buffer carrying a complete, valid meta reaches the application callback with

PROBE size=18446744073709551584      /* 96 - 128 */

Note on tooling: the over-read is not something a sanitizer catches here. filesrc allocates its blocksize and resizes the buffer down, so the 88-byte read stays inside a much larger live allocation; an ASAN build of main reports nothing and fails only on the assertion. The wrapped size is the part that is reliably observable, and that is what the tests assert.

The fix

The guarded form of the same operation, the way gst_tensor_meta_info_parse_memory() does it in nnstreamer:

1. The memory must be at least sizeof (GstTensorMetaInfo) before the parser touches it. That is exactly the window the parser fills — the parser reads the struct through a uint32_t * view of itself — so the bound is structural and does not depend on how meta versions are numbered.
2. The parse result decides whether the meta is used at all.
3. The header size the parsed meta reports must be non-zero and must fit the memory.

Step 3's zero check matters on its own. GST_TENSOR_META_VERSION_VALID only tests the 0xDE tag while GST_TENSOR_META_IS_V1 needs the version bits, so a meta can validate and still have no header size. Treating that as a zero-length header would hand the header bytes to the application as payload, described by an info the sender chose.

A rejected buffer takes the existing error: path: the memories are unmapped and unreffed, the data handle is destroyed, and no sink callback runs. The sink pad caps are deliberately left alone — unlike the tensor-count and size mismatches in the static branch, a malformed buffer is not a reason to renegotiate.

Tests

Six cases in nnstreamer_capi_flex, all driving a real pipeline through the public C API:

test	buffer	clause reached
sink_short_header_n	16 B	size < sizeof (GstTensorMetaInfo)
sink_invalid_magic_n	128 B, zeroed	parse_header returns FALSE
sink_unsupported_version_n	128 B, valid meta with the version number cleared	hsize == 0
sink_truncated_header_n	96 B, complete meta	hsize 128 > 96
sink_truncated_header_appsink_n	96 B, complete meta	same, through the appsink entry point
sink_header_only	exactly 128 B	accepted — callback once, tensor size 0

Which guard each case reaches was confirmed from the _ml_loge it triggers; the two guards log different messages, and within a guard the clause follows from the buffer size (16 < 88 against 128 ≥ 88 for the first, hsize 128 over a 96-byte memory against hsize 0 for the second).

One honest limit on that table: the first row is reached but not pinned. Deleting the size < sizeof (GstTensorMetaInfo) clause would let the 16-byte case fall through to the second guard, which still rejects it, so the suite would stay green. What that clause prevents — the parser reading past the memory — is not observable here for the reason given above: filesrc over-allocates, so the read stays inside a live allocation and no sanitizer fires. The clause is kept because the read is out of bounds of the buffer, which is what the sink is contractually given.

Headers are built with nnstreamer's own gst_tensor_meta_info_update_header() and truncated, so the tests follow the header layout rather than hardcoding it, and the unsupported version is produced by masking the real version tag rather than by writing a literal. They are fed in through filesrc with the flexible caps forced in front of the sink.

On main all five negative cases fail (result.received is 1, expected 0); with this change all six pass. sink_header_only passes before and after, so an over-strict guard would be caught too.

sink_truncated_header_appsink_n covers the second way into cb_sink_event — cb_appsink_new_sample() — which is what the ml-service extension, the training-offloading receiver and the Android JNI sink all end up using.

Verification

Built against nnstreamer 2.7.0 on Ubuntu 22.04, -Denable-ml-service=false:

unittest_capi_inference               242 tests   PASSED
unittest_capi_inference_single         48 tests   PASSED
unittest_capi_datatype_consistency      4 tests   PASSED

clang-format reports no diff on the test file, and GNU indent with the CI options reports no diff inside the changed function.

Gating: unittest_capi_inference runs on a PR through Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") only — pdebuild declares on: pull_request but has not actually been triggered by one since 2026-08-09. A regression in these cases blocks the merge through that job.

Review rounds

Four review passes, each by a fresh agent, and one approving review: round 1, round 2, round 3, round 4. Everything actionable is in the branch.

Round 2's two substantive findings drove the restructuring above:

• The parse_header return value had no coverage. Every negative test was short-circuiting on the size check, so deleting that clause would have left the suite green. sink_invalid_magic_n closes it, and moving the pre-parse bound from the header size (128) to sizeof (GstTensorMetaInfo) (88) means the 96-byte cases now exercise the second guard instead of the first.
• A validated meta can report hsize == 0. Round 1's reply had reasoned that zero was benign; it is not, and it is the same input class this PR exists to reject. Guarded, and sink_unsupported_version_n covers it.

Round 2 also corrected round 1's cur_hsize finding — the real hazard was the zero case, not a future larger header. sizeof (GstTensorMetaInfo) removes the version coupling that started that thread.

While covering the version case, sink_unsupported_version_n was found to be passing vacuously: it sized the file from the meta it had just mutated, so the header size came back 0 and filesrc pushed nothing. Fixed by taking the size before the mutation, and create_flex_header_file() now refuses a zero-length file so the same mistake cannot pass silently again.

Round 3 found no blocking issue. It verified sizeof (GstTensorMetaInfo) is 88 on every target this repo builds for — all members are uint32_t, including the anonymous union's, so there is no padding — and that it matches the parser's read window exactly. Its remaining points were the two body inaccuracies corrected above, plus the temp-directory boilerplate, which is now factored into create_flex_header_file () and remove_flex_header_file () (−17 lines net in the test file).

Round 4 re-checked the test refactoring against the head it replaced: the inputs that select each arm are unchanged byte for byte, including the ordering in sink_unsupported_version_n that the round-2 vacuous pass turned on, and the helper now cleans up its own directory when it fails, which removes a leak the earlier tests had on an assertion failure. No blocking issue.

Round 3 also independently confirmed what is noted under Verification below: pdebuild has not been triggered by a pull request since 2026-08-09 despite its on: pull_request, so the GBS x86_64 job is the only PR check that runs unittest_capi_inference. That dormancy is an infrastructure matter for its own issue, not something to fix here.

After the four rounds, an approving review (myungjoo-bot) raised four Low items on 277ec6b:

1. Log messages do not say which clause fired or by how much. Done — both _ml_loge calls now carry the tensor index and the sizes, e.g. (tensor 0, header 128 bytes, memory 96 bytes) for a truncated header and (tensor 0, header 0 bytes, memory 128 bytes) for an unsupported version.
2. create_flex_header_file () Doxygen misses size and the failure/ownership contract of dir. Done.
3. Document the drop in ml_pipeline_sink_cb. Not done. nnstreamer.h is the public native API header, and it has never said that the static branch drops buffers whose tensor count or size mismatch either; a note on the flexible case alone would read as if the static path delivered malformed buffers. This PR makes the existing contract — a valid pointer and a truthful size — hold rather than changing it. Documenting the drop belongs to a change that covers both branches.
4. G_STATIC_ASSERT between NNS_TENSOR_SIZE_LIMIT and ML_TENSOR_SIZE_LIMIT. Not done here. The two limits are both 256 today, but an assertion between them would not bound what reaches mem[]/map[]: num_tensors is taken from the buffer itself (gst_tensor_buffer_get_count (), which for a buffer at the 16-memory limit reads the count out of the extra-tensor header), so the question is whether that count is validated, not whether the constants agree. nnstreamer main now validates it — nnstreamer/nnstreamer@d4de7840 (item A1 of [Memory] Memory-safety audit of Tizen-built C/C++ components: tracking checklist nnstreamer#4920) rejects a header whose count exceeds the array — but that is not in a tagged release yet, and this repository does not require a minimum nnstreamer version, so a build against a current release still trusts the header. Whether the api should bound the count itself is a separate item from M9 and is being looked at on its own.

Two items are deliberately left out of this PR:

• The payload is still not checked against the info the header declares. sink_header_only documents that: the callback gets an info claiming 16 bytes over a 0-byte payload. That is a separate defect from M9, it applies to the sparse format differently, and fixing it here would risk rejecting legitimate flexible streams. It belongs in [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690 as its own item.
• The goto error at i > 0 has no test. The flex loop does not modify num_tensors and every memory is mapped before it runs, so the error: cleanup is index-independent and there is no distinct path to cover. Round 2 is right that tensor_mux/tensor_merge can produce multi-memory flexible buffers; what they cannot easily produce is one whose second memory is truncated, which is what such a test would need.

🤖 Generated with Claude Code

[myungjoo]

Note: This review was produced by a separate review agent (a Claude sub-agent) and is transcribed here by the PR author. It is an automated review, not a human maintainer's approval.

Verdict

The change resolves M9 of #690 correctly, is minimal, and is gated by CI. Findings below are all low severity; none are blocking. The PR is still DRAFT / DO NOT MERGE.

Does it solve the stated problem?

Yes, and it solves it the same way nnstreamer already solves it internally.

gst_tensor_meta_info_parse_memory() (nnstreamer_plugin_api_impl.c:1606-1632) is: init -> get_header_size -> msize < hsize bail -> parse_header. The new code in cb_sink_event() is the same sequence against map[i].size, so the api repo is no longer the odd one out.

Two details that make the guard actually sound rather than accidentally sound:

• gst_tensor_meta_info_init() (nnstreamer_plugin_api_util_impl.c:1458) sets magic/version, so GST_TENSOR_META_IS_VALID holds and gst_tensor_meta_info_get_header_size() returns 128 rather than 0. cur_hsize therefore can never degenerate to 0 and turn the check into a no-op.
• gst_tensor_meta_info_parse_header() reads val[0]..val[20], plus val[21] on the _NNS_TENSOR_FORMAT_SPARSE branch — 88 bytes at most, not 84 as the commit message says. Still well under 128, so the pre-check covers the whole read either way; only the narrative is off by one field.

Error-path handling is correct: at the point of the new goto error, all num_tensors memories are already mapped, and the error: block runs its own for (i = 0; ...), so the partially advanced loop index is harmless. _ml_tensors_data_destroy_internal (_data, FALSE) does not free the borrowed pointers. Leaving the sink pad caps alone is the right call — unlike the count/size mismatches in the static branch, a malformed buffer is not a reason to renegotiate, and nulling elem->sink per bad buffer would re-run caps discovery on every one.

Worth recording as a second, unstated benefit: on main, a failed parse left hsize = 0 and then called gst_tensor_meta_info_convert(), whose g_return_val_if_fail (gst_tensor_meta_info_validate (meta)) emits a GLib CRITICAL and leaves the nth GstTensorInfo zeroed. So that path delivered a buffer with an invalid ml_tensors_info_h and logged a critical. Both are gone now.

Regression risk

Low. The only newly-rejected inputs are memories shorter than the header they declare, which no valid flexible producer emits — a zero-payload tensor is exactly 128 bytes and still passes (sink_header_only pins that boundary). Cost on the streaming path is one extra comparison per tensor; gst_tensor_meta_info_init/get_header_size are hoisted out of the loop, so it is a wash. No other module is touched: 24 lines in one static function, the remaining 194 are tests.

Findings

1. (Low) cur_hsize bakes in "current meta version" as a lower bound.
If nnstreamer ever adds a meta version whose header is larger than 128, cur_hsize grows with it and this guard starts rejecting legitimate v1 memories sized between the two. gst_tensor_meta_info_parse_memory() has the identical trait, so the behaviour is at least consistent — but none of the four tests would catch it, because they all derive their sizes from gst_tensor_meta_info_get_header_size() on a current meta and would move in lockstep with the change. The PR's own claim that the second check is "currently unreachable" rests on the same assumption. Worth one comment line naming the assumption explicitly.

2. (Low, coverage) Every test uses a single-memory buffer.
The guard lives inside the per-tensor loop, and the part of it that is easiest to break in a future refactor — goto error taken at i > 0 while memories 0..num_tensors-1 are all mapped — is not exercised. A case with two memories where only the second is truncated would cover the loop/unmap interaction and would fail loudly if someone later changed the error: cleanup to unmap only i memories.

3. (Low, test robustness) create_flex_header_file() drops the return of gst_tensor_meta_info_update_header().
That call is g_return_val_if_fail (gst_tensor_meta_info_validate (meta), FALSE). If a future nnstreamer change made {_NNS_INT32, dimension {4,0,...}} fail validation, the helper would silently write a zero-filled buffer, all three _n tests would keep passing for the wrong reason, and only sink_header_only would flip. A single ASSERT_TRUE (or a g_assert) on that return makes the whole set self-checking.

4. (Info) Negative tests wait on a fixed g_usleep (300000) with no positive signal.
Taken alone, sink_short_header_n / sink_truncated_header_n / sink_truncated_header_appsink_n can pass vacuously on a slow runner if the buffer never reached the sink at all. This is adequately mitigated by sink_header_only, which drives the identical filesrc ! other/tensors,format=flexible ! tensor_sink shape and requires a callback — so a pipeline that stops delivering, or caps that stop being parsed as flexible, still fails the suite. Please keep those four tests together so the mitigation is not lost to a later reorganisation.

5. (Nit) Commit message: "84 bytes" -> up to 88 bytes (the val[21] sparse field). No effect on the fix.

Future-change detection and CI gating

Confirmed the tests actually block a merge:

• debian/rules override_dh_auto_test -> packaging/run_unittests.sh ./tests, which propagates the gtest exit code, run by the pdebuild Ubuntu 22.04 workflow on every pull_request to main.
• packaging/machine-learning-api.spec:429 runs ./tests/capi/unittest_capi_inference in the GBS build.

The unittest_capi_inference meson timeout is 100 s; the four new cases add roughly 1.5 s, so there is no timeout pressure.

For detecting breakage introduced by other modules: if a future nnstreamer change altered the flexible header layout, gst_tensor_meta_info_convert, or the caps handling such that cb_sink_event no longer takes the flexible branch, sink_header_only fails (the 128-byte buffer would mismatch the static-branch size check and be dropped). That is the right shape of canary. The gap is the multi-tensor case in finding 2.

Documentation

No documentation change is required and none is missing. cb_sink_event() is static; there is no public API, ABI, enum, or error-code change, and no architectural change. The ml_pipeline_sink_cb contract is unchanged — the change makes the existing contract (a valid ml_tensors_data_h with a truthful size) actually hold. Optionally, the sink-register documentation could state that malformed flexible buffers are dropped without invoking the callback, but that is a nicety rather than an omission.

[myungjoo]

Note: This is a second-round review, produced by a separate review agent (a Claude sub-agent) working from the current force-pushed head 4e49a3c, the round-1 comment (#694 (comment)), the nnstreamer sources, and this repository's CI configuration. It is an automated review, not a human maintainer's approval.

Verdict

The three items marked Addressed are genuinely in the tree and are correct. Of the two Not changed items, one rebuttal is technically sound but rests on a reachability claim that looks wrong, and the other answers a different question than the one that was asked — though neither is a reason to block.

Two findings that round 1 missed matter more than either of those, and both sit on the changed lines:

• N1 — the new map[i].size < hsize check is a no-op when hsize == 0, which a validated meta can legitimately return. The PR body's justification ("either 128 or 0, both <= 128") is precisely where the hole is.
• N2 — all three _n tests are rejected by the same condition. The "return value is dropped" arm named in M9 is never executed by the test suite; gst_tensor_meta_info_parse_header() is short-circuited away in every negative test.

Both are cheap to fix. The PR remains DRAFT. Recommendation: not yet mergeable, on N2 primarily (a guard whose main arm is untested) and N1 secondarily.

---

1. Verification of the three "Addressed" items

(a) create_flex_header_file() now checks gst_tensor_meta_info_update_header() — confirmed, tests/capi/unittest_capi_inference.cc:8255. Correct: content is g_malloc0 (MAX (size, hsize)), so update_header's 128-byte memset plus 88-byte memcpy (sizeof (GstTensorMetaInfo) = 3x4 + 16x4 + 4 + 4 + 4) stay in bounds even for size == 16; on failure content is freed and NULL returned, no leak, and path is only built inside the success branch. Every caller does ASSERT_TRUE (file != NULL).

Nit: the new control flow conflates two failure modes — an invalid meta and a failed g_file_set_contents now both surface as file == NULL. The ASSERT_TRUE/g_assert directly on the update_header return that round 1 suggested would have named the cause. Not worth a respin on its own.

(b) waitPipelineStateChange (handle, ML_PIPELINE_STATE_PLAYING, 2000) — confirmed, :8294. This is a stronger signal than the PR body claims, and it introduces no timing regression:

• tensor_sink and appsink are GstBaseSink subclasses with async = TRUE. The pipeline does not reach PLAYING until preroll completes, and preroll here completes on the first buffer. So reaching PLAYING proves the buffer reached the sink's preroll path — not merely that the source ran.
• waitPipelineStateChange (tests/capi/unittest_util.c:133) polls ml_pipeline_get_state every 10 ms and returns as soon as the state matches, so in the healthy case it costs ~10-30 ms, not 2 s. ml_pipeline_get_state uses gst_element_get_state (..., GST_MSECOND); a mid-ASYNC transition yields the current state and the poll continues, so there is no spurious ML_ERROR_UNKNOWN. After EOS the pipeline stays PLAYING, so the wait cannot hang on the short files used here.
• It runs before the 300 ms settle window, so it cannot shorten it.
• It converts a hard state-change failure (e.g. filesrc cannot open the file) from a silent vacuous pass into an explicit ML_ERROR_STREAMS_PIPE.

It does not fully retire round-1 finding 4: preroll proves the buffer arrived, not that cb_sink_event ran to completion, so the 300 ms is still load-bearing for the EXPECT_EQ (result.received, 0U) assertions and sink_header_only is still the real control. But it is a real improvement.

(c) 84 -> 88 bytes — confirmed in the commit body of 4e49a3c, and consistent with gst_tensor_meta_info_parse_header() reading val[0]..val[20] plus val[21] on the sparse branch.

---

2. The two "Not changed" rebuttals

2a. cur_hsize bakes in the current meta version — the reply is non-responsive, and the real hazard is the opposite one

Round 1's scenario was a future meta version whose header is larger than 128, which would make cur_hsize reject legitimate v1 memories. The reply answers a version whose header shrinks. Those are different failures, and the second does not address the first.

More usefully: round 1's scenario cannot actually occur without someone editing gst_tensor_meta_info_get_header_size() itself, because that function returns 128 only for GST_TENSOR_META_IS_V1 and 0 for everything else. Adding a v2 today would make cur_hsize become 0, not 256 — which turns the first check into a no-op rather than an over-strict one. So the concrete hazard from meta versioning is the hsize == 0 case, not the oversized one. That is finding N1 below, and the reply's supporting sentence — "a validated meta reports either 128 or 0, both <= the 128 the first check already required" — is exactly the assumption that does not hold up.

The inline comment (/* the parser reads a whole header of the current version before validating one */) does name the assumption, so I would not block on the documentation half of round-1 finding 1.

2b. The goto error at i > 0 — technically sound, but the reachability claim is asserted rather than shown

The core of the rebuttal is correct and I verified it: the flex loop does not touch num_tensors, all num_tensors memories are mapped by the earlier loop, and error: runs its own for (i = 0; i < num_tensors; i++). There is genuinely no distinct cleanup path to cover. Agreed, non-blocking.

The claim I do not accept as written is "not reachable through the public API without adding an artificial element." On inspection, stock nnstreamer elements appear to produce exactly that buffer:

• tensor_mux / tensor_merge advertise { static, flexible } on both sink and src pads (gst/nnstreamer/elements/gsttensor_mux.c:89,94).
• gst_tensor_time_sync_buffer_from_collectpad() (nnstreamer_plugin_api_impl.c:458-509) appends each collected pad's memories into one output buffer and, when the src pad is flexible and the input already was, does not re-append a header.
• A truncated single-memory flexible buffer passes through gst_tensor_buffer_from_config() untouched: the header-splitting loop terminates with num == 1, so it takes out = gst_buffer_ref (in).

So filesrc(valid) ! other/tensors,format=flexible ! mux.sink_0 filesrc(96B) ! other/tensors,format=flexible ! mux.sink_1 tensor_mux name=mux ! tensor_sink is a stock-element candidate for a two-memory flexible buffer whose second memory is truncated. I did not build it — collectpads behaviour with untimestamped filesrc buffers is the open question — so please treat this as unproven, not proven reachable. The point is only that "not reachable" should not be stated as a fact on the strength of the argument given.

Given 2b costs little either way, I would spend the coverage budget on N2 instead, which is strictly more valuable.

---

3. New findings

N1. (Medium-low, on the changed lines) hsize == 0 passes the second check, and the header is then delivered as tensor payload

GST_TENSOR_META_VERSION_VALID(v) is ((v) & 0xDE000000) == 0xDE000000 — it checks only the top byte. GST_TENSOR_META_IS_V1(v) additionally requires bit 12 ((v & 0x00FFF000) & 0xDE001000 reduces to v & 0x00001000).

Therefore a header with magic = 0xfeedcced, version = 0xDE000000, and otherwise plausible type / dimension / format / media_type:

• passes gst_tensor_meta_info_validate(), so gst_tensor_meta_info_parse_header() returns TRUE;
• but gst_tensor_meta_info_get_header_size() returns 0, because bit 12 is clear.

Walking the new code with a 128-byte memory of that shape:

map[i].size (128) < cur_hsize (128)          -> false
!gst_tensor_meta_info_parse_header (...)     -> false   /* validates fine */
hsize = 0
map[i].size (128) < hsize (0)                -> false   /* no-op */
gst_tensor_meta_info_convert (...)           -> TRUE
_data->tensors[i].data = map[i].data + 0
_data->tensors[i].size = map[i].size

The application receives the 128 header bytes as tensor payload, together with an ml_tensors_info_h whose type and dimension are entirely attacker-chosen. This is not memory-unsafe on its own (pointer and size stay inside the mapping) and it is not a regression — main does the same thing — but it is exactly the input class this PR exists to reject: hsize == 0 means "a meta version this build cannot parse", and the PR body currently reasons that 0 is benign because 0 <= 128.

One extra term closes it, and also makes the second check reachable (and therefore testable) instead of provably dead:

if (hsize == 0 || map[i].size < hsize) {

N2. (Medium, coverage) Three of the four tests exercise the same single condition; the parse-failure arm has zero coverage

cur_hsize is 128. The negative tests use 16-byte and 96-byte buffers — both < 128. Because the guard is written as

if (map[i].size < cur_hsize || !gst_tensor_meta_info_parse_header (&meta, map[i].data))

the || short-circuits, so gst_tensor_meta_info_parse_header() is never called in sink_short_header_n, sink_truncated_header_n, or sink_truncated_header_appsink_n. All three are rejected by map[i].size < cur_hsize alone.

Consequences:

• The half of M9 phrased as "gst_tensor_meta_info_parse_header(...) return value ignored" — the arm the PR body describes as "Check the parse result" — is not covered by any test. Deleting || !gst_tensor_meta_info_parse_header (...) would leave all four tests green.
• The second map[i].size < hsize check is not covered either (it is unreachable today, per the N1 analysis).
• sink_truncated_header_n is functionally a duplicate of sink_short_header_n; sink_truncated_header_appsink_n adds only the second entry point, not a second condition. The PR body's table reads as though the 96-byte cases exercise "header size 128 > 96" as a distinct check — they do not; that is the first check.

The fix costs one test and no new machinery: a 128-byte file that is not a valid header (e.g. all zeros, so magic == 0). That gives map[i].size >= cur_hsize and parse_header() == FALSE, covering the arm. It is also a meaningful canary against main, where the same input takes the hsize = 0 path and trips gst_tensor_meta_info_convert()'s g_return_val_if_fail, emitting a GLib CRITICAL and delivering a zeroed GstTensorInfo — the second, unstated improvement round 1 noted, which is currently untested. create_flex_header_file() already zero-fills, so this only needs a flag (or a small sibling helper) that skips update_header.

If N1 is fixed, a second variant with version = 0xDE000000 covers the hsize == 0 arm for the same cost.

N3. (Low; suggest a follow-up item on #690 rather than a change here) The guard bounds the header, not the payload — and sink_header_only pins that

create_flex_header_file() writes type = _NNS_INT32, dimension[0] = 4, so gst_tensor_meta_info_get_data_size() is 16 while the payload after the header is 0 bytes. The callback therefore gets size == 0 alongside an ml_tensors_info_h for which ml_tensors_info_get_tensor_size() returns 16. An application that sizes its copy from info — a normal thing to do — reads 16 bytes past the end of the mapped memory. It is the mirror image of M9: M9 was size far larger than reality; this is info larger than size.

Two things make this more than theoretical:

• nnstreamer's own gst_tensor_buffer_from_config() treats exactly this shape as "Failed to get tensor buffer, data size is mismatched." (nnstreamer_plugin_api_impl.c, the offset + mem_size[i] > total guard), so this repository is the permissive one here.
• A header-only flexible memory is always inconsistent, because gst_tensor_dimension_is_valid() requires rank >= 1 — so the boundary sink_header_only pins as "the smallest valid flexible tensor" cannot correspond to a well-formed tensor at all.

I am not asking for this in this PR: gst_tensor_meta_info_get_data_size() is the check, but tightening it risks rejecting producers that pad, and it is outside M9. Please file it as a follow-up item on #690, and consider not describing sink_header_only as "accepted, callback once, tensor size 0 — correct" without that caveat, since the test locks the permissive behaviour in place.

N4. (Nit) Per-buffer _ml_loge on a malformed stream

A steady stream of malformed flexible buffers now logs at frame rate. The static branch does the same, so this is consistent — but the static branch at least stops re-deriving caps by nulling elem->sink. Not a change request; noting it because the flex branch deliberately (and, I agree, correctly) does not.

N5. (Nit, pre-existing pattern) Temp directory leaked on assertion failure

If ASSERT_TRUE (dir != NULL) or ASSERT_TRUE (file != NULL) fires, the g_mkdtemp directory and any file in it are left on disk — the four tests have no scope guard. The same pattern already exists at :329, :623, :974, :7673, so this is not introduced here.

---

4. Everything else checked

• No regression introduced by the round-1 response. waitPipelineStateChange is a net improvement with no timing risk (section 1b); the new create_flex_header_file() control flow is leak-free and bounds-correct (section 1a).
• Style / CI. Static checks is green. I re-ran clang-format 16 with the repository .clang-format over the whole new test block — no diff. Line lengths in the changed C hunk are in line with the rest of the file. At the time of writing the GBS and Android x86_64/arm64 jobs are still pending on this force-push; Static checks, DCO, Spell Check, build (x86) and build (armeabi-v7a) have passed.
• CI gating is unchanged from round 1 and still adequate: debian/rules -> packaging/run_unittests.sh ./tests on the pdebuild Ubuntu 22.04 pull_request job, and ./tests/capi/unittest_capi_inference in the GBS unit_test 1 build. The new cases add roughly 1.5 s against a 100 s meson timeout, and waitPipelineStateChange adds bounded tens of milliseconds in the healthy case.
• Blast radius. 22 net lines in one static function; no other module touched. gst_tensor_meta_info_init / get_header_size are hoisted, so the streaming cost is one comparison per tensor. Code size is proportionate to the fix.
• Regression on valid inputs. The only newly rejected inputs are memories shorter than 128 bytes, which no conforming flexible producer emits. Nothing else in this repository depends on cb_sink_event accepting short flexible memories.
• Documentation. Agree with round 1: cb_sink_event() is static, there is no public API/ABI/enum/error-code or architectural change, so nothing is missing. If N3 is ever acted on, the ml_pipeline_sink_register contract would need a sentence, since dropping buffers silently becomes a documented behaviour rather than an implementation detail.

Summary of requested changes

1. N2 (please address): add one 128-byte invalid-header case so the !gst_tensor_meta_info_parse_header() arm — the half of M9 about the ignored return value — is actually executed by the suite.
2. N1 (please address): reject hsize == 0 explicitly; the current second check is a no-op for a validated non-V1 meta, and the PR body's reasoning about it should be corrected.
3. 2a / 2b (please correct the record; no code change needed): the cur_hsize rebuttal answers the shrinking case rather than the growing one, and the growing case is not the real hazard anyway; and the "not reachable through the public API" claim about multi-memory flexible buffers is unproven and looks wrong given tensor_mux's { static, flexible } pads.
4. N3 (follow-up on [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690, not this PR): the guard bounds the header, not the payload, and sink_header_only pins a header/payload mismatch as valid.

[myungjoo]

Note: This is a third-round review, produced by a separate review agent (a Claude sub-agent) working from the current force-pushed head 20ea3ac, the round-1 (comment) and round-2 (comment) comments, the nnstreamer sources, and this repository's CI configuration. It is an automated review, not a human maintainer's approval.

Verdict

Both round-2 blockers (N1, N2) are genuinely fixed, and the restructuring is better than the minimal patch either finding asked for. I found no correctness, safety, or regression defect on the changed lines. Nothing here is blocking.

Mergeable, with one gating condition and one correction to the record:

• The Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") job is still pending. It is the only CI job on this PR that executes unittest_capi_inference — see "CI gating" below, where I have to correct both earlier rounds. Merge after it goes green and the DRAFT / DO NOT MERGE marks are lifted.

Remaining findings are two nits on the PR narrative and three pre-existing test-style nits. None require a respin.

---

1. Focus item 1 — is sizeof (GstTensorMetaInfo) the right pre-parse bound?

Yes, and it is exact — not merely conservative. Derived rather than assumed:

gst_tensor_meta_info_parse_header() (nnstreamer_plugin_api_util_impl.c:1612) reads, through a uint32_t * view of the caller's pointer:

read	indices	end offset
magic, version, type	val[0..2]	12
memcpy (meta->dimension, &val[3], 4 * NNS_TENSOR_RANK_LIMIT)	val[3..18] (rank limit = 16)	76
format, media_type	val[19], val[20]	84
sparse_info.nnz, on the _NNS_TENSOR_FORMAT_SPARSE branch only	val[21]	88

GstTensorMetaInfo (tensor_typedef.h:310-326) is uint32_t magic; uint32_t version; uint32_t type; tensor_dim dimension; uint32_t format; uint32_t media_type; union { GstSparseTensorInfo sparse_info; }; = 4 + 4 + 4 + 64 + 4 + 4 + 4 = 88 bytes.

Padding/alignment: every member is uint32_t (tensor_dim is uint32_t[16]; the anonymous union's only member is a one-uint32_t struct). Alignment is 4 for every member and for the aggregate, all offsets are already 4-aligned, and there is no tail padding. sizeof is therefore 88 on every ABI this repo targets — x86, x86_64, armeabi-v7a, arm64-v8a, aarch64, armv7l. There is no ILP32/LP64 divergence to worry about, and no #pragma pack dependence.

So the guard's window and the parser's read window coincide exactly, byte for byte. Round 1's cur_hsize (128) was merely sufficient; sizeof is tight, and the tightness is what makes the second guard reachable (finding N2's real fix — see §3).

Durability. The PR body says the bound "does not depend on how meta versions are numbered". That is correct and is the property that mattered — the whole round-1/round-2 cur_hsize thread is now moot, because the bound is no longer derived from GST_TENSOR_META_IS_V1. One nuance for the record: the exactness holds at NNS_TENSOR_RANK_LIMIT == 16. If the rank limit grew, sizeof grows to (6 + rank) * 4 while the fixed val[19..21] reads stay at 88, so the bound stays sound (just no longer tight). If it ever shrank below 16, parse_header() would already be reading past its own struct — an nnstreamer-internal bug, not something this guard could be expected to absorb. Framed as "the size of the struct the parser fills", the bound is correct by construction. No change wanted.

Comparison with nnstreamer. gst_tensor_meta_info_parse_memory() bounds with hsize (128) before parsing, so it is stricter up front. This PR is deliberately looser up front (88) and catches the 88..127 window in the second guard instead. Net accept/reject behaviour is identical; the only difference is that parse_header is now allowed to run on an 88..127-byte memory, which is safe by the table above. Confirmed intentional and correct.

2. Focus item 2 — is the hsize == 0 guard correct?

Yes, and it cannot reject a valid stream.

• GST_TENSOR_META_VERSION_VALID(v) is ((v) & 0xDE000000) == 0xDE000000 (top byte only); GST_TENSOR_META_IS_V1(v) additionally requires the major-version nibble. gst_tensor_meta_info_get_header_size() returns 128 for V1 and 0 for anything else, including metas that pass gst_tensor_meta_info_validate(). Round 2's N1 analysis is confirmed verbatim against the source.
• Every conforming producer writes its header through gst_tensor_meta_info_update_header(), which g_return_val_if_fails on gst_tensor_meta_info_validate() and whose only in-tree source of version is gst_tensor_meta_info_init() -> GST_TENSOR_META_VERSION = 0xDE001000. Parsed back, that is V1, hsize = 128. hsize == 0 is reachable only from a hand-forged or foreign-version header, which is exactly the input class this PR exists to reject. No false-rejection path exists.
• The old behaviour with hsize == 0 was to hand the 128 header bytes to the application as payload, described by an attacker-chosen type/dimension. Closing it is right, and sink_unsupported_version_n pins it.

Also checked: gst_tensor_meta_info_convert()'s return is still dropped, but it is now provably TRUE — its g_return_val_if_fail (gst_tensor_meta_info_validate (meta)) re-runs a pure predicate on the same unmodified meta that parse_header() already returned TRUE for. No latent CRITICAL remains on this path.

3. Focus item 3 — do the six tests actually reach their branches?

I walked each case through the guard with the real constants (sizeof = 88, hsize = 128 or 0). All six reach distinct, intended arms; none is vacuous.

test	file	size < 88?	parse_header	hsize	arm actually taken
sink_short_header_n	16 B	yes	not called (short-circuit)	—	guard 1, size clause
sink_invalid_magic_n	128 B all-zero	no	FALSE (magic == 0)	—	guard 1, parse clause
sink_unsupported_version_n	128 B, version = 0xDE000000	no	TRUE	0	guard 2, hsize == 0 clause
sink_truncated_header_n	96 B, complete meta	no (96 >= 88)	TRUE	128	guard 2, size < hsize clause
sink_truncated_header_appsink_n	96 B	no	TRUE	128	same, via cb_appsink_new_sample()
sink_header_only	128 B valid	no	TRUE	128	accepted

Spot-checks that the table depends on:

• sink_unsupported_version_n really produces a validating non-V1 meta. GST_TENSOR_META_VERSION = GST_TENSOR_META_MAKE_VERSION(1,0) = 0xDE001000; & 0xFF000000 = 0xDE000000. VERSION_VALID -> true; IS_V1 -> false. gst_tensor_meta_info_validate() still passes (magic ok, type = _NNS_INT32 < _NNS_END, dimension {4,0,...} rank-1 valid, format = _NNS_TENSOR_FORMAT_STATIC, media_type = _NNS_TENSOR), so update_header() writes the file rather than bailing. Inside update_header, hsize is 0 so the memset is a no-op — harmless, because content is already g_malloc0'd at 128. Correct by construction, not by luck.
• create_flex_header_file()'s size == 0 guard. This is the fix for the vacuous pass the PR body describes, and it is placed correctly: it fires before the allocation, so a zero-length file can never be written, and every caller does ASSERT_TRUE (file != NULL). content = g_malloc0 (MAX (size, hsize)) keeps update_header's 88-byte memcpy in bounds even for size == 16, and g_file_set_contents then truncates to size. No leak on either exit (content is freed unconditionally; path is only built in the success branch and cleared on write failure).
• init_flex_meta() usage. Called by four of the six tests; sink_invalid_magic_n deliberately does not use it (it passes meta = NULL to get a zeroed file) and instead uses a locally init'd meta only to obtain the 128-byte header size. Consistent, and the _NNS_INT32 / dimension[0] = 4 choice is what makes validate() pass in the three cases that need it to.
• Non-vacuity chain. sink_header_only is a genuine control, not a decorative one: it drives the identical filesrc ! other/tensors,format=flexible ! tensor_sink shape and requires received == 1 and result.size == 0. result is initialised {0U, 1}, so the size assertion cannot pass by omission — the callback must actually run and must actually observe a zero-length tensor. If caps ever stopped being parsed as flexible (get_tensors_info_from_caps -> found == FALSE -> goto error before the flex branch), or buffers stopped being delivered at all, sink_header_only fails loudly while the five _n tests would silently go green. Verified ml_tensors_data_get_tensor_data() returns ML_ERROR_NONE for a zero-size tensor (c/src/ml-api-common.c), so that assertion is meaningful rather than accidentally skipped.
• waitPipelineStateChange (PLAYING) is a real signal here. gst_tensor_sink_class_init sets render/render_list but never calls gst_base_sink_set_async_enabled (FALSE), so async preroll is on and PLAYING is not reached until a buffer has arrived at the sink. That converts "the file never opened / nothing was pushed" from a silent vacuous pass into an explicit ML_ERROR_STREAMS_PIPE. It is checked before the 300 ms settle window, so it does not shorten it.
• No double-count risk in sink_header_only. tensor_sink emits SIGNAL_NEW_DATA only from gst_tensor_sink_render_buffer(); there is no preroll vfunc override, and ml_pipeline_construct connects only new-sample (not new-preroll) for appsink. So exactly one callback per buffer — EXPECT_EQ (result.received, 1U) is stable, not flaky-by-one.
• The "fails on main" claim holds for all five negative cases. On main, each of them either wraps map[i].size - hsize or takes the hsize = 0 path into gst_tensor_meta_info_convert()'s g_return_val_if_fail; either way the callback still runs and result.received is 1. Verified by walking the pre-patch code, not taken on trust.

T1 (Low, informational — no change requested)

The new map[i].size < sizeof (GstTensorMetaInfo) clause is reached by sink_short_header_n, but it is not pinned: delete that clause and the suite still goes green. A 16-byte filesrc buffer lives inside a 4096-byte allocation that gst_buffer_resize shrank, so the 88-byte over-read stays in a live block; parse_header then either returns FALSE on garbage or (if the block is a fresh zero page, which is the common case) returns TRUE with hsize = 128, and guard 2 rejects at 16 < 128. Either way received stays 0 and the test passes.

This is the exact mirror image of round 2's N2, now applying to the clause that round 2's fix introduced. Unlike N2, it is inherent rather than an oversight: the clause is a memory-safety bound, and no observable behaviour distinguishes it from guard 2 under filesrc. Making it observable would need an exactly-sized allocation (e.g. appsrc + gst_buffer_new_wrapped) and an ASAN build — which is precisely the combination the PR body already documents as not working through filesrc.

I am not asking for that test. The clause's correctness is a static property (88 == sizeof == the parser's maximum read), established above and stable under the only realistic evolution of the struct. I raise it only so the PR's table is not read as claiming more than it delivers — see T3.

4. Focus item 4 — regressions introduced by the rewrite

None found.

• Accept/reject behaviour on valid inputs is unchanged. The only newly rejected classes are: memories under 88 bytes, memories whose meta fails validate(), metas that validate but are not V1, and memories under the 128-byte header they declare. No conforming flexible producer emits any of these; a zero-payload flexible tensor is exactly 128 bytes and is explicitly pinned as accepted.
• Streaming cost. One comparison against a compile-time constant, plus the existing get_header_size() call, per tensor per buffer. The round-1/round-2 hoisting of gst_tensor_meta_info_init/get_header_size out of the loop is gone, but it was only compensating for a call the current form no longer makes — the new form does less per-iteration work than either previous version. No _ml_loge on the success path.
• Error path. Unchanged and still correct: at both goto error sites all num_tensors memories are already mapped by the earlier loop, the flex loop never writes num_tensors, and error: runs its own for (i = 0; i < num_tensors; i++). _ml_tensors_data_destroy_internal (_data, FALSE) does not free borrowed pointers. Not nulling elem->sink remains the right call.
• Blast radius. 24 changed lines in one static function; grep confirms cb_sink_event is the only consumer of gst_tensor_meta_info_parse_header/get_header_size in the whole repository, so there is no sibling copy of the bug and no other module to regress. The appsink entry point is covered by sink_truncated_header_appsink_n, which matters because ml-service, the training-offloading receiver and the Android JNI sink all reach cb_sink_event that way.
• Test-suite stability / runtime. Six tests x (~300 ms settle + setup); the waitPipelineStateChange poll costs ~10-30 ms in the healthy case and only reaches its 2 s cap on a genuine failure. ~2-3 s added against a 100 s meson timeout (and the GBS path invokes the binary directly, so no per-test timeout applies there). No shared state between the cases — each creates its own g_mkdtemp directory, so they are order- and parallel-safe.

5. Focus item 5 — does the PR body match the code?

Substantially yes. I re-derived every load-bearing number (88, 128, 0xDE001000, val[21], the per-test guard column) and they all check out, including the 18446744073709551584 = 96 - 128 figure and the "all five negative cases fail on main" claim. The "Review rounds" section describes what actually happened, and the two deferred items are honestly scoped. Two narrative nits:

T2 (Nit) The _ml_loge methodology proves less than stated

Each negative case was confirmed to reach the guard it is named for by checking which _ml_loge fires

There are two distinct messages ("...header is invalid" / "...does not fit the memory") covering four arms. The log therefore distinguishes guard 1 from guard 2, but not sink_short_header_n from sink_invalid_magic_n, nor sink_unsupported_version_n from sink_truncated_header_n. The within-pair distinction is established by construction (16 < 88 cannot reach the parser; a 128-byte zeroed file cannot fail the size clause), which is sound — it is just not what the sentence claims. Either soften the sentence or, if you want the stronger property mechanically, give the two clauses of each guard their own message.

T3 (Nit) The table's first row overstates coverage

sink_short_header_n's "guard reached" cell is accurate, but per T1 the test does not fail if that guard is removed. Suggest a footnote to that effect, so a future reader does not delete the clause on the strength of a green suite.

6. Focus item 6 — CI gating (correcting rounds 1 and 2), size, docs

C1 (Info, but please act on it before merging) pdebuild does not gate this PR

Rounds 1 and 2 both asserted that debian/rules -> packaging/run_unittests.sh ./tests runs on the "pdebuild Ubuntu 22.04 pull_request job". That job is not running on this PR. gh run list --commit 20ea3ac returns only four workflows — Tizen/GBS, Spell Check, Static checkers, Android Build Test — and gh run list --workflow pdebuild.yml --limit 30 shows ["schedule"] as the only trigger event across its entire recent history, with no run at all since 2026-08-09, despite the on: pull_request: branches: [main] stanza in the file (present on origin/main, so it is not a base-branch problem).

Consequence: the sole PR check that executes unittest_capi_inference is Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") (packaging/machine-learning-api.spec:429), and it is still pending as I write this. Everything else on the PR is a compile-only or lint job.

So the six new tests are gated, but by one job rather than two, and the redundancy both earlier rounds assumed is not there. Please confirm that job green before merging. (The dormant pdebuild PR trigger looks like a separate repo-infrastructure issue worth a standalone report — it is not this PR's problem.)

Current status: Static checks, DCO, Spell Check, and all four Android build (*) jobs pass. The two unit_test 0 GBS jobs are compile-only.

Code size

24 changed lines in c/src/ml-api-inference-pipeline.c for the fix; 285 added lines of test. The ratio is high but the tests cover four distinct rejection arms plus two entry points plus one positive boundary, so it is earned rather than padded.

T4 (Nit, optional): the six tests repeat the same ~13 lines of g_build_path / g_mkdtemp / g_remove / g_free boilerplate. A small create_flex_temp_dir() helper or a gtest fixture would cut roughly 40 lines and remove six copies of the same cleanup sequence. The file's existing tests do the same thing, so this is consistency-preserving as written — mentioning it only as an option, not a request.

Pre-existing test-style nits (not introduced here, no action needed)

• T5: result->received is written by the callback under G_LOCK (callback_lock) but read unlocked by wait_pipeline_process_buffers and EXPECT_EQ. Technically a data race that TSAN would flag; identical to the existing *count_sink pattern throughout this file.
• T6: the g_mkdtemp directory and its file leak if ASSERT_TRUE (dir != NULL) or ASSERT_TRUE (file != NULL) fires — same as :329, :623, :974, :7673.

Documentation

Unchanged conclusion from rounds 1 and 2, re-verified: cb_sink_event() is static; no public API, ABI, enum, or error-code change; the ml_pipeline_sink_cb contract is not altered, only made to actually hold. Nothing is missing. If the deferred payload-vs-header item is ever acted on, ml_pipeline_sink_register's documentation would then need a sentence about buffers being dropped silently — but that belongs with that change, not this one.

On the two deferred items

Both are correctly scoped out and the reasoning given is sound:

• Payload not checked against the declared dimension — genuinely a separate defect from M9, genuinely differs for the sparse format, and sink_header_only now carries a @note saying exactly what it does and does not pin. That is the right way to defer it. Please do file it on [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690.
• goto error at i > 0 — I agree there is no distinct cleanup path to cover, for the reason given in §4. The revised wording ("what they cannot easily produce is one whose second memory is truncated") is a fair and accurate retreat from round 1's "not reachable through the public API", which round 2 was right to challenge.

---

Summary

No blocking findings. N1 and N2 are properly closed, and the chosen fix — moving the pre-parse bound to sizeof (GstTensorMetaInfo) — is better than either finding asked for, because it makes the second guard reachable and therefore testable instead of dead. The six tests hit six distinct outcomes with a working non-vacuity control.

Recommendation: merge, once Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is green (per C1 it is the only job that runs these tests) and the DRAFT / DO NOT MERGE marks are lifted. T1-T6 are optional and can be folded into a later commit or dropped entirely; only T2/T3 (two sentences in the PR body) are worth touching before merge, and neither affects the code.

[myungjoo]

Note: This is a fourth-round review, produced by a separate review agent (a Claude sub-agent) working from the current force-pushed head 277ec6b, the round-1 (comment), round-2 (comment) and round-3 (comment) comments, the nnstreamer sources, and this repository's CI configuration. It is an automated review, not a human maintainer's approval.

Verdict

No blocking findings, and nothing that warrants a respin. The test refactor is behaviour-preserving: all six cases still reach the same six distinct arms with the same inputs, and the one ordering that previously caused a vacuous pass is intact. The new helper's resource handling is correct on every exit path and is a small improvement on what round 3 flagged as T6. The PR body's two corrected sentences now match the code.

Merge condition (unchanged from round 3's C1, and now stronger): Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is still pending. Because this delta is entirely in tests/capi/unittest_capi_inference.cc, that job is now the only check on the PR that even compiles the changed lines — see the CI section. Merge after it goes green and the DRAFT / DO NOT MERGE marks are lifted. No fifth round is needed.

---

0. What actually changed since round 3

git diff 20ea3ac 277ec6b is one file, tests/capi/unittest_capi_inference.cc, +52 / −69 = −17 net. That matches the body's "−17 lines net in the test file" exactly.

c/src/ml-api-inference-pipeline.c is byte-identical to the round-3 head. So round 3's sections 1 (sizeof (GstTensorMetaInfo) is the exact 88-byte parser window), 2 (hsize == 0 cannot reject a valid stream), 4 (no regression, error path, blast radius) and 6 (docs) carry over verbatim; I spot-checked rather than re-litigated them and found nothing to revise. This review is therefore about the test refactor and the body.

1. Focus item 1 — do the six tests still reach their intended arms?

Yes. Every input that determines the arm is bit-identical before and after the refactor.

test	meta	file size	sink	expected	arm	changed?
sink_short_header_n	init_flex_meta	16	tensor_sink	0	guard 1, size clause	no
sink_invalid_magic_n	NULL (zeroed)	128	tensor_sink	0	guard 1, parse clause	no
sink_unsupported_version_n	init_flex_meta, version &= 0xFF000000U	128	tensor_sink	0	guard 2, hsize == 0	no
sink_truncated_header_n	init_flex_meta	96	tensor_sink	0	guard 2, size < hsize	no
sink_truncated_header_appsink_n	init_flex_meta	96	appsink	0	same, via cb_appsink_new_sample()	no
sink_header_only	init_flex_meta	128	tensor_sink	1	accepted	no

The four properties the earlier rounds established as load-bearing all survive:

• The ordering that fixed round 2's vacuous pass is intact. sink_unsupported_version_n still does hsize = gst_tensor_meta_info_get_header_size (&meta); before meta.version &= 0xFF000000U;, then passes that saved hsize as the size. Moving the dir argument to the end of the parameter list did not disturb the statement order, which is the only thing that mattered here.
• The zero-length backstop is intact and still ahead of every side effect. if (size == 0) return NULL; is still the second statement of create_flex_header_file(), now before g_mkdtemp(), so a size-0 call cannot even create a directory, let alone a file. Every caller still ASSERT_TRUE (file != NULL).
• The non-vacuity control is intact. sink_header_only still initialises result = { 0U, 1 } and still asserts both received == 1 and size == 0, so the size assertion cannot pass by omission. Round 3's argument — that this case fails loudly if caps stop parsing as flexible or buffers stop being delivered, while the five _n cases would silently go green — is unaffected.
• run_flex_header_pipeline() is untouched, so waitPipelineStateChange (PLAYING) still fires before the 300 ms settle window and still converts "nothing was pushed" into an explicit failure rather than a silent pass.

No new vacuous-pass surface is introduced. The refactor removed the ASSERT_TRUE (dir != NULL) from each test, but that assertion is not lost — a g_mkdtemp() failure now makes the helper return NULL, which the surviving ASSERT_TRUE (file != NULL) catches. Every way the helper can fail (size 0, mkdtemp, update_header returning FALSE, g_file_set_contents failing) funnels into that one NULL, so no failure mode can now run an empty or wrong-sized pipeline and score a pass.

I also confirmed the six new TEST names do not collide with the pre-existing nnstreamer_capi_flex cases (sink_multi at :8075, src_multi at :8140), and that G_LOCK_DEFINE_STATIC (callback_lock) (:76) and wait_pipeline_process_buffers (:62) are the file's existing definitions, not new ones.

2. Focus item 2 — create_flex_header_file() / remove_flex_header_file()

Correct on every path. No leak, no double free, no uninitialised read, no overrun.

Out-param initialisation. *dir = NULL; is the first statement, ahead of both early returns. So *dir is defined on all five exits — size == 0, mkdtemp failure, update_header FALSE, set_contents failure, and success. The callers' gchar *dir, *file; are consequently never read uninitialised, including in the ASSERT_TRUE failure case (where the test returns before touching dir at all).

Ownership, exit by exit:

exit	tmpdir	content	path	*dir
size == 0	not created	not allocated	—	NULL
g_mkdtemp fails	g_free	not allocated	—	NULL
update_header FALSE	g_remove + g_free	g_free	never built	NULL
g_file_set_contents fails	g_remove + g_free	g_free	g_clear_pointer	NULL
success	transferred to *dir	g_free	returned	tmpdir

content is freed unconditionally; tmpdir is transferred only in the path != NULL branch, so there is exactly one owner in every case. file != NULL implies dir != NULL by construction (*dir = tmpdir is guarded by if (path), and tmpdir is non-NULL there because g_mkdtemp returned it).

Buffer bound is sound. content = g_malloc0 (MAX (size, hsize)) where hsize comes from a freshly init'd _meta and is therefore 128. gst_tensor_meta_info_update_header() (nnstreamer_plugin_api_util_impl.c:1590-1603) writes memset (header, 0, get_header_size (meta)) followed by memcpy (header, meta, sizeof (GstTensorMetaInfo)) — at most max (128, 88) = 128 bytes, since get_header_size() returns only 128 (V1) or 0. So the 16-byte and 96-byte cases write into a 128-byte allocation and g_file_set_contents then truncates to size. In bounds, including for sink_unsupported_version_n, where the meta's own header size is 0 and the memset is a harmless no-op over already-zeroed memory.

Informational only: that bound is derived from a default meta's header size, not from the caller's meta. It is correct today because 128 is the maximum get_header_size() can return, but it is an implicit coupling. Not worth changing in a test helper — recording it so it is not rediscovered as a surprise.

remove_flex_header_file() is correct. File first, then directory — the required order, since g_remove on a non-empty directory fails. g_remove does handle directories: POSIX remove() dispatches to rmdir() for them, and GLib's Win32 path falls back to _wrmdir after _wremove. Both pointers are freed. It has no NULL guard, but it is unreachable with NULL given the ASSERT_TRUE (file != NULL) in all six callers and the file implies dir relation above.

This is a net improvement on round 3's T6. Previously, ASSERT_TRUE (file != NULL) firing leaked fullpath and left the g_mkdtemp directory behind in /tmp. Now the helper removes and frees its own directory before returning NULL, so that path leaks nothing. The residual T6 case — an ASSERT_* inside run_flex_header_pipeline() aborting the test before remove_flex_header_file() runs — still exists and is still identical to the pattern at :329, :623, :974, :7673. No action.

3. Focus item 3 — does the PR body match the code now?

Yes, both round-3 corrections landed accurately, and I re-derived every load-bearing figure independently.

• T2 is properly fixed. The body now says the two guards log different messages and that the within-guard clause "follows from the buffer size", spelling out all four discriminators (16 < 88 vs 128 >= 88; hsize 128 over 96 bytes vs hsize 0). That is exactly what the code and the six inputs support, and it no longer claims the log distinguishes clauses it cannot distinguish.
• T3 is properly fixed, and honestly. "the first row is reached but not pinned", with the correct reason (filesrc over-allocates, so the over-read stays inside a live block and no sanitizer fires) and the correct consequence (deleting the clause leaves the suite green). I confirmed this independently: a 16-byte file yields a buffer holding the first 16 valid header bytes, and without the size clause parse_header would read val[0..20] out of the surrounding live allocation, after which guard 2 rejects at 16 < 128 either way. The paragraph claims no more than it delivers.
• -17 lines net matches git diff --stat exactly (52 insertions, 69 deletions).
• clang-format — I re-ran clang-format 16.0.6 with the repo's .clang-format over the whole post-refactor test file: zero diff. The body's formatting claim survives the refactor. (The indent claim is trivially still true; the C file did not change.)
• Re-checked and still holding: 88, 128, 0xDE001000, val[21], 18446744073709551584 = 96 − 128, the six-row table, and "all five negative cases fail on main".

Two editorial nits on the body, neither affecting the code:

B1 (Nit) The "Review rounds" section still opens with a stale count

Two review agents have gone over this.

The section then goes on to describe round 3 (and after this comment, round 4). Suggest "Three review agents" / "Four review rounds", or just drop the count.

B2 (Nit) The Verification block's local numbers predate this refactor

242 / 48 / 4 PASSED was measured on the pre-refactor head. The refactor is test-only and does not change the test count, so the numbers remain correct in substance — but the compile-and-run evidence for 277ec6b currently comes from CI rather than from the quoted local run. Either re-run locally or lean explicitly on the GBS job; not worth blocking on, given the CI section below.

4. Other findings (all Low, none requiring action)

• L1 (doc). create_flex_header_file()'s Doxygen documents meta, dir and @return, but not size. @param dir says "The caller should free it" without noting that it is set to NULL on failure or that remove_flex_header_file() is the intended disposer; remove_flex_header_file() documents neither that it takes ownership nor that it frees both arguments. Cosmetic, and Static checks is green on this head.
• L2 (optional simplification). dir is never used for anything but cleanup, and it is always g_path_get_dirname (file). A single-argument remove_flex_header_file (gchar *file) that derives the directory itself would remove the out-param, its NULL-initialisation question, and one declaration from each of the six tests. Purely a taste call — the explicit out-param is equally defensible, and I am not asking for it.
• L3 (pre-existing, unchanged). Round 3's T5 stands: result->received is written under G_LOCK (callback_lock) but read unlocked by wait_pipeline_process_buffers and EXPECT_EQ. Identical to the *count_sink pattern used throughout this file. No action.
• L4 (pre-existing, unchanged). The pipeline string interpolates the temp path into location="%s". Spaces in TMPDIR are fine inside the quotes; a " or \ would not be. Same as every other file-driven test in this file; not introduced here.

Also confirmed as non-issues: g_mkdtemp's template ends in exactly six X; glib/gstdio.h is already included (:12) so g_remove is declared; the redundant (gchar *) cast on g_mkdtemp is correctly gone; no new unused-variable or maybe-uninitialised warning surface; each test still gets its own g_mkdtemp directory, so the suite stays order- and parallel-safe.

5. CI gating on this head

Round 3's C1 is re-verified and has grown more important, because this delta is entirely inside tests/capi/unittest_capi_inference.cc:

• The Android build (*) jobs build the Android bindings and do not compile the C-API unit tests.
• The two unit_test 0 GBS jobs build the package without the test suite.
• Static checks, Spell Check and DCO are lint/metadata jobs.

So Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is the only check on this PR that compiles the changed file at all, let alone runs the six tests. A typo in the new helper would be invisible everywhere else.

Status as I finish: DCO, Spell Check, Static checks, build (x86), build (x86_64), build (armeabi-v7a) pass; build (arm64-v8a), both unit_test 0 GBS jobs, and the gating unit_test 1 GBS job pending.

---

Summary

The refactor does exactly what it claims and nothing more: −17 lines, six tests with identical inputs reaching six identical arms, the round-2 ordering fix and the zero-length backstop both preserved, no new vacuous pass, and cleaner failure-path cleanup than before. create_flex_header_file() and remove_flex_header_file() are leak-free and bounds-correct on every path. The two body sentences round 3 asked for are corrected, and every number in the body re-derives.

No remaining issues. Recommendation: merge, once Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is green — per the CI section it is the only job that compiles or runs any of this delta — and the DRAFT / DO NOT MERGE marks are lifted. B1, B2 and L1-L4 are optional and can be folded in later or dropped; none of them touches shipped behaviour, and none justifies another round.

[myungjoo-bot]

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

Summary: The PR adds three checks to the flexible-tensor loop in cb_sink_event (c/src/ml-api-inference-pipeline.c:364-382): map[i].size >= sizeof (GstTensorMetaInfo) before parsing, use of the gst_tensor_meta_info_parse_header result, and hsize != 0 && hsize <= map[i].size before the pointer/size arithmetic; a rejected buffer takes the existing error: path with sink caps untouched. The defect is confirmed on main 8952123: the parser is called unchecked, its return is ignored, and map[i].size - hsize is gsize arithmetic, so a 96-byte valid-meta buffer hands the app a size of 2^64-32. Verified against the nnstreamer source (v2.6.0-209): GstTensorMetaInfo is 3*u32 + u32[16] + 2*u32 + union{u32} = 88 bytes, all uint32_t, no padding on any target; parse_header reads exactly those 88 bytes before calling validate; GST_TENSOR_META_VERSION_VALID tests only the 0xDE byte while GST_TENSOR_META_IS_V1 additionally needs bit 12, so version = 0xDE000000 validates yet get_header_size returns 0 — the zero check is necessary, not defensive; gst_tensor_meta_info_parse_memory uses the same guard shape. CI: all 10 checks pass, and the GBS x86_64 unit_test 1 log (job 102310634336) shows all 8 nnstreamer_capi_flex tests OK including the six new ones. merge-tree is clean. The actionable items from the four relayed reviews (hsize == 0, parse-arm coverage, the vacuous sink_unsupported_version_n, helper cleanup on failure) are all present on head 277ec6b.

Also verified: all num_tensors memories are mapped in the first loop and the flex loop never modifies num_tensors, so error: unmaps/unrefs exactly 0..num_tensors-1, _ml_tensors_data_destroy_internal (_data, FALSE) does not free borrowed pointers, and no callback runs — no leak, no double-unmap; the static branch is untouched; cb_appsink_new_sample still returns GST_FLOW_OK, so a dropped buffer does not error the pipeline, consistent with the static branch; leaving caps alone is correct since a malformed buffer says nothing about negotiated caps; hsize <= map[i].size (zero payload) is pinned by sink_header_only with result initialised so the size assertion cannot pass by omission; each negative test reaches the claimed arm by construction (16 < 88; 128 zeroed -> parse FALSE; version-masked -> hsize 0; 96 -> 128 > 96) and all five fail on main (callback runs once, received == 1); create_flex_header_file refuses size == 0 and cleans its dir on every failure exit; helper linking is unchanged (unittest_common_dep -> nnstreamer_dep, and the file already used nnstreamer util calls). Single commit, DCO present, message accurate. Approving; the items below are non-blocking.

1. [Low] c/src/ml-api-inference-pipeline.c:367-369, 376-378: the two _ml_loge messages name the element but not the tensor index, map[i].size, or hsize, so a field log cannot tell which clause fired or how short the memory was. Suggest appending " (tensor %u, memory %" G_GSIZE_FORMAT " bytes, header %" G_GSIZE_FORMAT " bytes)".
2. [Low] tests/capi/unittest_capi_inference.cc:8250-8253 (create_flex_header_file Doxygen): @param size is undocumented and @param dir does not say it is NULL on failure / disposed by remove_flex_header_file.
3. [Low, optional] c/include/nnstreamer.h:127-136 (ml_pipeline_sink_cb): one sentence noting that buffers with a malformed flexible-tensor header are dropped without invoking the callback. Not required; the change only makes the existing contract (valid pointer, truthful size) hold.
4. [Low, pre-existing, out of scope] ml-api-inference-pipeline.c:296: gst_tensor_buffer_get_count can return up to NNS_TENSOR_SIZE_LIMIT (256) and mem[] / map[] are sized ML_TENSOR_SIZE_LIMIT (256); currently safe, but the two constants are independent definitions — worth a G_STATIC_ASSERT in a follow-up.

No back-door or suspicious behavior found: the product change is 19 lines of bounds checking with no new calls or success-path behaviour; test helpers only create/remove files under g_mkdtemp.

[myungjoo]

Note: This is a fifth-round review, produced by a separate review agent (a Claude sub-agent) working from the current force-pushed head 35b4348, the four earlier review comments, the approving review by myungjoo-bot on 277ec6b, the nnstreamer sources, and a local build of this head. It is an automated review, not a human maintainer's approval.

Verdict

No blocking findings. The 277ec6b..35b4348 delta is correct, and both "Not done" rebuttals hold up in substance. The only remarks are two wording points in the PR body's item-4 paragraph (see 3b). Neither touches the code, and neither needs another round.

Mergeable once the pending builds are green and the DRAFT / DO NOT MERGE marks are lifted. This delta touches c/src, so the GBS unit_test 0 jobs now matter too, not just the unit_test 1 job (see section 5).

---

1. The delta: git diff 277ec6b 35b4348

The diff is 2 files, +8/−6: the two _ml_loge calls in cb_sink_event () and the create_flex_header_file () Doxygen. Nothing else moved.

Format and argument types

After _ml_detail expands, both calls have the format "%s:%s:%d: " "<message>", with __FILE__, __func__, __LINE__ in front of the caller's arguments.

message	conversion	argument	declared type
both	[%s]	elem->name	gchar *
both	tensor %u	i	guint (:289)
guard 1	memory %" G_GSIZE_FORMAT "	map[i].size	gsize (GstMapInfo.size)
guard 2	header %" G_GSIZE_FORMAT "	hsize	gsize (:360)
guard 2	memory %" G_GSIZE_FORMAT "	map[i].size	gsize

The conversions match the arguments in number, order and type. G_GSIZE_FORMAT is the right choice over the %zu used elsewhere in this file for size_t. GLib only promises that gsize is the same width as size_t, not the same type, while glibconfig.h defines G_GSIZE_FORMAT to match gsize exactly. So on LP64 (x86_64, aarch64) it is "lu" for unsigned long, and on ILP32 (armv7l, armeabi-v7a, x86) it is "u" for unsigned int.

Guard 1 does not print the header size, and that is correct. On that path hsize has not been assigned yet (it is uninitialised on the first iteration), and the parse either did not run or failed. Adding it, as the approving review's suggested string did, would have logged an indeterminate value.

Combination with _ml_detail and the -Werror set

• _ml_detail(fmt, ...) (ml-api-internal.h:431) expands to "%s:%s:%d: " fmt, __FILE__, __func__, __LINE__, ##__VA_ARGS__. The new fmt argument is "...%" G_GSIZE_FORMAT " bytes).". The commas inside the literals do not split macro arguments, and G_GSIZE_FORMAT expands to a string literal. The whole format therefore stays a single concatenated literal, which is what -Wformat-nonliteral / -Wformat-security / -Werror=format=2 need.

• Adjacent string-literal concatenation is C89, so the change is fine under -std=gnu89. The GNU-isms here (##__VA_ARGS__, __func__ in gnu89) are pre-existing and used throughout the file.

• I checked all four backends. They are selected at ml-api-internal.h:32-65 and the format reaches each one unchanged:

  • Linux: g_critical → g_log (G_GNUC_PRINTF).
  • Tizen: dlog_print (DLOG_ERROR, tag, ...).
  • Android: __android_log_print (prio, tag, ...) (__printflike(3, 4)).
  • FAKEDLOG: set only in tests/meson.build for the test objects, not for the library.

  Every backend receives the same format and argument list after its tag, so the type analysis above applies to all of them.

Verified by building, not only by reading

• Build: local build of 35b4348 with gcc 11.4, using the project's own flags (-std=gnu89 -Wall -Werror -Werror=format=2 -Wformat-nonliteral -Wformat-security ..., confirmed from ninja -t commands). It is warning-free.

• Negative control: replacing the G_GSIZE_FORMAT on :369 with "u" makes the same build fail with ml-api-internal.h:431:30: error: format '%u' expects argument of type 'unsigned int', but argument 9 has type 'gsize' {aka 'long unsigned int'} [-Werror=format=]. So format checking really does reach through _ml_loge → g_critical → g_log. The committed version passes because it is right, not because nothing checks it.

• Tests: unittest_capi_inference 242 PASSED, unittest_capi_inference_single 48 PASSED, unittest_capi_datatype_consistency 4 PASSED (nnstreamer 2.7.0, Ubuntu 22.04). The Verification block in the body is therefore also current for this head, which closes round 4's B2.

• Observed log lines from the five negative cases (path prefix trimmed):

  cb_sink_event:367: ... header is invalid (tensor 0, memory 16 bytes).                                  <- sink_short_header_n
  cb_sink_event:367: ... header is invalid (tensor 0, memory 128 bytes).                                 <- sink_invalid_magic_n
  cb_sink_event:377: ... does not fit the memory (tensor 0, header 0 bytes, memory 128 bytes).           <- sink_unsupported_version_n
  cb_sink_event:377: ... does not fit the memory (tensor 0, header 128 bytes, memory 96 bytes).          <- sink_truncated_header_n
  cb_sink_event:377: ... does not fit the memory (tensor 0, header 128 bytes, memory 96 bytes).          <- sink_truncated_header_appsink_n
  

  The two examples quoted in the body's "Review rounds" item 1 match this output exactly. There is also a side benefit: the log alone now separates all four rejection arms (memory < 88 vs >= 88 in guard 1; header 0 vs header > memory in guard 2). What round 3's T2 said the old messages could not show, the new ones do.

• Formatting: GNU indent with the options in .github/workflows/static.check.scripts/indent.sh produces no hunk inside cb_sink_event () (the two hunks it does produce are elsewhere in the file and pre-existing). clang-format produces no diff on the test file. The new 84- and 88-column Doxygen lines are not reflowed because .clang-format has CommentPragmas: '\* @'. Static checks is green on this head.

2. Doxygen of create_flex_header_file () vs. what the helper does

Every claim matches the code at unittest_capi_inference.cc:8253-8293:

• @param size "truncated or zero-padded to it". content = g_malloc0 (MAX (size, hsize)) with hsize = 128, then g_file_set_contents (path, content, size, NULL). If size < 128 the header is cut; if size > 128 the tail is zero from g_malloc0. With meta == NULL the whole file is zero, which @param meta already says.
• @param dir "NULL on failure". *dir = NULL is the first statement, ahead of every exit. *dir = tmpdir happens only in the if (path) branch. On each of the four failure exits (size == 0, g_mkdtemp, update_header FALSE, g_file_set_contents) *dir stays NULL, and the temp directory is removed and freed in the helper.
• @return "Release both with remove_flex_header_file ()". That function does g_remove (file), g_remove (dir) (file first, as required), then g_free on both. This is the intended disposer, and the doc now says so. It also resolves round 4's L1 from the creating side.

The one thing left implicit is that size == 0 is refused. It is covered by "NULL on failure", so it is not worth a respin.

3. The two "Not done" rebuttals

3a. Item 3 (document the drop in ml_pipeline_sink_cb): sound

I checked c/include/nnstreamer.h. Neither ml_pipeline_sink_cb (:126-136) nor ml_pipeline_sink_register () (:296-325) says anything about buffers being dropped. That is also true for the static branch's count and size mismatches, which have dropped silently all along. A sentence covering only the flexible case would indeed suggest the static path delivers malformed buffers. The public header is the Tizen native API contract, so this should be one change covering both branches, not an asymmetric note in this fix. Agree.

3b. Item 4 (G_STATIC_ASSERT between the two limits): sound in substance, two wording points

I read the three functions in C:/Users/myung/nnstreamer, which is checked out at 804bbe81 (v2.6.0-209):

• gst_tensor_buffer_get_count () (nnstreamer_plugin_api_impl.c:1912-1950): if gst_buffer_n_memory () < NNS_TENSOR_MEMORY_MAX (16) it returns that. Otherwise it maps the last memory, and if gst_memory_map_is_extra_tensor () holds it returns extra_info->num_extra_tensors + NNS_TENSOR_MEMORY_MAX. num_extra_tensors is a raw uint32_t from buffer data, and the guint sum can even wrap.
• gst_memory_map_is_extra_tensor () (:53-70) checks only map->size >= sizeof (GstTensorExtraInfo) and magic == NNS_TENSOR_EXTRA_MAGIC. It does not check the version, and it does not bound num_extra_tensors against NNS_TENSOR_SIZE_EXTRA_LIMIT (240). grep finds no other place that bounds it.
• gst_tensor_buffer_get_nth_memory () (:1686-1757) takes that count as its index bound and walks extra_info->infos[] and gst_memory_share () offsets derived from reserved / infos[], again without comparing them to the mapped size.

So for that source, "gst_tensor_buffer_get_count () trusts the extra-tensor header" is accurate. The approving review's premise, that the count "can return up to NNS_TENSOR_SIZE_LIMIT (256)", holds only for well-formed producers. A G_STATIC_ASSERT that the two constants agree would therefore not bound mem[] / map[] (cb_sink_event () :287-288), or _data->tensors[], against a forged header. The body's conclusion, "the question is whether that count is validated, not whether the constants agree", is the right one. Not fixing it here is correct: it sits ahead of the M9 lines, in the first mapping loop, and is a different defect.

Two precision points on that paragraph. Neither changes the conclusion, and I am not asking for a code change.

1. "for more than 16 memories" cannot happen. A GstBuffer holds at most 16 memories: GST_BUFFER_MEM_MAX, which NNS_TENSOR_MEMORY_MAX mirrors per tensor_typedef.h:45-52. The header is consulted once the buffer is at 16 memories, which is how more than 16 tensors are represented. Suggest "for more than 16 tensors" or "once the buffer holds 16 memories".
2. The sentence is accurate for the nnstreamer the reasoning was based on, but stale for nnstreamer main. nnstreamer/nnstreamer main has since landed d4de7840 "[Common] Do not trust the extra-tensor header of a gst-buffer" (committed 2026-09-07). gst_memory_map_is_extra_tensor () there now rejects num_extra_tensors > NNS_TENSOR_SIZE_EXTRA_LIMIT, so with that commit the count is at most 16 + 240 = 256, and get_nth_memory () bounds every offset and size against the map. No tag contains d4de7840 yet. This repository does not pin a minimum nnstreamer (meson.build:32, dependency('nnstreamer') with no version). The body's conclusion therefore still holds for the nnstreamer versions this API can be built against, and an API-side check of num_tensors against ML_TENSOR_SIZE_LIMIT is still what closes it there. ml_pipeline_sink_cb's own doc (nnstreamer.h:132, "the maximum number of tensors is #ML_TENSOR_SIZE_LIMIT") gives that check its contract. The follow-up should also note that, with d4de7840, a static assert that NNS_TENSOR_SIZE_LIMIT <= ML_TENSOR_SIZE_LIMIT does become a real bound, so the two are complementary. Suggest qualifying the sentence along the lines of "which, before nnstreamer d4de7840, trusts the extra-tensor header".

4. Anything new

Nothing. On the success path, behaviour is unchanged: the only edits are to strings and arguments of two _ml_loge calls that run only just before goto error. No control flow, no new calls, and no new per-buffer cost on accepted buffers. Round 2's N4, about per-buffer logging on a malformed stream, stands as it was and was already accepted as consistent with the static branch. The Doxygen change has no effect on the build. Single commit, DCO present, and Spell Check is green.

5. CI on this head

Unlike 277ec6b, this delta touches c/src/ml-api-inference-pipeline.c, so the build-only jobs are now meaningful for it:

• Tizen GBS build on Ubuntu (armv7l, --define "unit_test 0") is the only job that compiles the new format strings under meson's -Werror set on an ILP32 target, through the real dlog_print. aarch64 does the same for LP64 on Tizen.
• The Android build (*) jobs compile the file (Android-nnstreamer.mk:33) but without -Werror (LOCAL_CFLAGS := -O3 -fPIC ...), so they would only warn. armeabi-v7a and x86, the two 32-bit ABIs, have already passed.
• Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is still the only job that runs unittest_capi_inference.

Status when I finished: DCO, Spell Check, Static checks, build (armeabi-v7a) and build (x86) pass. build (arm64-v8a), build (x86_64) and all three GBS jobs are pending.

---

Summary

• Delta: the format strings and arguments match on every backend and ABI. Proven by a clean -Werror=format=2 build plus a negative control that fails as expected. The log output matches the body verbatim, and now distinguishes all four arms.
• Doxygen: now accurate on size, padding, failure state and disposal.
• Rebuttal 3: correct.
• Rebuttal 4: correct about the nnstreamer source it cites. Its wording should say "tensors" rather than "memories", and should acknowledge nnstreamer d4de7840, which now bounds the count on main. It is still out of M9's scope.

Recommendation: merge, once Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") and the two GBS unit_test 0 builds are green, and the DRAFT / DO NOT MERGE marks are lifted. The two body-wording points in 3b are optional, and no sixth round is needed.

[myungjoo-bot]

Automated re-review of head 35b4348 (delta since the previously approved 277ec6b; verified directly by the review process, no separate agent this round).

Summary: The delta is 2 files, +8/-6: the two _ml_loge calls in the flexible branch of cb_sink_event now report the tensor index and sizes, and the create_flex_header_file Doxygen documents @param size, the NULL-on-failure state of dir, and disposal via remove_flex_header_file (). Both close the Low items from the previous review. Checked on the PR head: i is guint (%u), hsize and map[i].size are gsize (G_GSIZE_FORMAT), so the conversions match on LP64 and ILP32; _ml_detail keeps the format a single concatenated literal, so -Werror=format=2 still applies; the first guard deliberately omits hsize, which is unassigned on that path. The guards themselves, the error: path, and the success path are byte-identical to 277ec6b. The log now distinguishes all four rejection arms (memory < 88 vs parse failure; header 0 vs header > memory), which is what the earlier review asked for.

CI on 35b4348: all 10 checks pass, including the GBS armv7l / aarch64 build lanes that compile the new format strings under Tizen's -Werror set on ILP32 and LP64. The GBS x86_64 unit_test 1 job (102763088316) shows all 8 nnstreamer_capi_flex tests OK — sink_short_header_n, sink_invalid_magic_n, sink_unsupported_version_n, sink_truncated_header_n, sink_truncated_header_appsink_n, sink_header_only at ~301 ms each — and 246 tests passed in unittest_capi_inference. Single commit, DCO present, merge-base is current main. The author's decision to leave the ml_pipeline_sink_cb doc note and the G_STATIC_ASSERT out of this PR is reasonable: the former should cover both branches in one change, and the latter does not bound a forged extra-tensor header on nnstreamer releases without d4de7840, so an API-side num_tensors check is the real follow-up. Approval stands; nothing further requested.

No back-door or suspicious behavior found.