Repository navigation
Conversation
|
이 리뷰는 별도 컨텍스트의 리뷰 에이전트가 작성한 것을 옮겨 적은 것입니다. (This review was written by a separate code-review agent in an isolated context and is being relayed here as-is.) Scope of this reviewReviewed against #690 item H4 (Android JNI CustomFilter output size mismatch → Does it fix H4? — Yes, verified
Regression check across the 4 callers of
|
|
리뷰 감사합니다. 각 항목에 대한 대응입니다 (49e23a5). LOW-MEDIUM —
|
|
이 리뷰는 별도 컨텍스트의 리뷰 에이전트가 작성한 것을 옮겨 적은 것입니다. (This review was written by a separate code-review agent in an isolated context and is being relayed here as-is.) 이것은 이전 리뷰(#695#issuecomment-5594506694)에 대한 재리뷰이며, 작성자 대응(49e23a5) 반영 후 전체 diff를 다시 직접 코드로 검증했습니다. 이전 리뷰 지적사항 해소 여부 — 모두 확인됨(Verified)각 항목을 실제 코드로 직접 재확인했습니다.
추가로 직접 검증한 사항 (이번 재리뷰에서 새로 확인)
결론이전 리뷰에서 제기된 4개 항목 중 3개(Javadoc, 죽은 assertion, FLEXIBLE 반박)는 코드로 확인한 결과 정확하고 타당하게 처리되었습니다. 나머지 1개(Android instrumented test가 CI에서 실행되지 않음)는 사실이지만 이 PR의 스코프를 벗어나는 저장소 전반의 기존 구조적 한계이며, 이를 보완하기 위해 CI에서 실제로 실행되는 C 유닛테스트를 추가한 것으로 충분히 완화되었다고 판단합니다. 새로운 이슈는 발견되지 않았습니다. 리뷰 시점 기준 Blocking 이슈 없음(No blocking issues found). |
myungjoo-bot
left a comment
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated review (transcribed from an AI review agent's report; please verify before acting).
Summary: The PR makes nns_parse_tensors_data() distinguish a handle it creates (allocate + clone, as before) from one the caller passes in (fill in place; tensor count and every size must match, else the parse fails), and bounds the Java tensor count by ML_TENSOR_SIZE_LIMIT (256, matching NNStreamer.TENSOR_SIZE_LIMIT) before any index. The defect is confirmed on main: nns_customfilter_invoke() (nnstreamer-native-customfilter.c:125) passes the non-NULL out from ml_pipeline_custom_invoke() (c/src/ml-api-inference-pipeline.c:2846-2848, tensors[i].data = out[i].data, released with free_data = FALSE), and main's clone branch did g_clear_pointer (&data->tensors[i].data, g_free) on a size mismatch, g_malloced unseen replacements for extra Java tensors, and wrote data->num_tensors unbounded. All four call sites were checked: pipeline.c:651, service.c:360, singleshot.c:237/245 initialise the handle to NULL right before the call, so their behaviour is byte-equivalent (data is always NULL from _ml_tensors_data_create_no_alloc); the custom-filter path is the only caller-owned one. In nnstreamer, a negative invoke return maps to GST_FLOW_ERROR plus a posted invoke-failure application message (tensor_filter.c:955-974), consistent with "nothing reaches the sink and the pipeline reports an error". No local-ref leaks in the new error paths; jsize -> guint is safe; non-direct buffers cannot reach GetDirectBufferAddress because TensorsData.checkByteBuffer rejects them on insert. merge-tree is clean, all 10 checks pass, and the x86_64 unit_test 1 log shows nnstreamer_capi_custom.invoke_output_buffer_p OK (368 ms), 241 tests passed. All three commits are DCO-signed. Approving; the items below are wording-level.
- [Low] Commit message
94fae0b(and PR text): "Each of them hit the invalid free before the previous commit" is inaccurate for two of the three Android cases. On base, the extra-tensor case leaks ag_malloced buffer per invoke (tensor[0] matched, tensor[1] was NULL so it was allocated, never freed); the closed/empty case setnum_tensors = 0, skipped the loop, returned TRUE and delivered an unfilled frame to the sink. Only the larger-buffer case hits the invalid free. All three still fail before / pass after, so the tests are valid; suggest "each of them corrupted or silently mis-handled the framework's output buffer". - [Low]
CustomFilter.java:38-39Javadoc: "the buffer is dropped and the pipeline reports an error" understates the effect.GST_FLOW_ERRORfrom tensor_filter puts the pipeline in the error state (the new tests themselves catchIllegalStateExceptionon subsequentinputData/stop), so it is not a per-buffer drop like returningnull. Suggest "the invoke fails and the pipeline stops with an error". - [Low]
nnstreamer-native-api.c:594-596(PR side): the!clonebranch still doesdata->tensors[i].data = data_ptreven whencreated == FALSE, which would silently replace a caller-owned pointer. No caller does(clone = FALSE, pre-existing handle)today, so it is a latent trap rather than a bug; ag_return_val_if_fail (created || clone, FALSE)or a note in the new@paramdoc would lock the contract in. - [Low, informational] The three
APITestCustomFilter_ntests need a device and do not run in CI (android.ymlbuilds the AAR only); the C test covers the framework-side contract, not the JNI code itself. Pre-existing repo gap, acceptable here.
No back-door or suspicious behavior found.
nns_parse_tensors_data() may be given a data handle that already holds buffers. The only such caller is nns_customfilter_invoke(), where the handle wraps the output memory of tensor_filter: it is mapped GstMemory, not a GLib allocation, and the framework unmaps and keeps using it after the callback returns. The clone path treated those buffers as its own. When the TensorsData returned by CustomFilter.invoke() was built from a tensors info other than the registered output info, the sizes did not match, so the mapped memory was passed to g_free() and a replacement was allocated. That is an invalid free (heap corruption), the framework never sees the replacement, and GStreamer later unmaps memory this code already released. A Java list longer than the registered output info allocated and leaked a buffer on every invoke, one longer than ML_TENSOR_SIZE_LIMIT wrote past the fixed tensors array, and an empty list (a closed TensorsData) passed the parse with nothing copied, so an unfilled frame went downstream. Split the two ownership cases. A handle created here still allocates and clones as before. A handle given by the caller is only filled in: the number of the tensors and every buffer size must match, and a mismatch now fails the parse so the custom-filter invoke returns an error instead of corrupting the heap. The tensor count is bounded in both cases. A caller-owned handle is accepted only together with clone, because the direct-access path would overwrite the caller's pointers. No caller does that today; the check keeps it that way. Fixes the item H4 of nnstreamer#690. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Android negative cases drive a CustomFilter whose invoke() returns a TensorsData that does not match the registered output info. Before the previous commit each of them mishandled the output memory of the framework: the larger buffer passed the mapped memory to g_free(), the extra tensor allocated and leaked a buffer on every invoke, and the closed (empty) object passed the parse with nothing copied, so an unfilled frame reached the sink. Now the invoke fails for all three and nothing reaches the sink. The C unittest locks down what the JNI binding relies on: the invoke callback of a custom-easy filter is handed an output handle whose tensor count and buffer sizes come from the registered output info, with the memory already allocated by tensor_filter, and the data written into that memory is what the sink receives. It runs in CI, so a change that hands the callback a differently owned output buffer fails there rather than only on a device. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Say in CustomFilter.Callback.invoke() that the returned data is copied into the output memory of the pipeline, so it has to match the output information passed to create(), and that a mismatch fails the invoke and stops the pipeline with an error. That is not the per-buffer drop of returning null: tensor_filter turns a failed invoke into GST_FLOW_ERROR. The requirement is now enforced by the JNI layer, so it belongs in the API documentation. Also drop the assertions on mInvalidState from the new negative cases: they run with a sink callback that only counts, so the flag can never be set and the check cannot fail. The number of the received data is the assertion that matters. The remaining hunk reflows the new unittest to what clang-format wants. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
49e23a5 to
7d792b7
Compare
|
Response to the approving review (#pullrequestreview-5151019871). All four items were checked against the code; three are addressed in this PR. The series was rewritten ( 1. [Low] "Each of them hit the invalid free" is wrong for two of the three cases — agreed, fixed. 2. [Low] The Javadoc understates the effect — agreed, fixed. 3. [Low] The 4. [Low, informational] androidTest is not in CI — acknowledged, no change. Relayed on behalf of the author; the review above was written by an AI review agent. |
|
이 코멘트는 별도 컨텍스트의 리뷰 에이전트가 작성한 리뷰를 옮겨 적은 것입니다. 이것은 재리뷰이며, myungjoo-bot의 APPROVED 리뷰(4개 Low)와 그에 대한 작성자 대응(49e23a5, 이후 394979f/baadfb5/7d792b7로 이력 재작성)을 모두 읽고, 실제 코드를 직접 읽어 검증했습니다. 추측인 부분은 명시적으로 "추측"이라고 표기했습니다. 이력 재작성(rebase) 검증 — 트리 차이 확인됨
이전 지적 1~3 해소 여부 — 모두 코드로 직접 검증, 해소 확인(1) 커밋 메시지의 케이스별 서술이 base 동작과 일치하는가 — 일치함.
(2) (3) 새
H4(#690) 자체가 해결되었는가 — 해결됨
회귀 / 스코프 / 테스트 — 새로운 이슈 없음
결론새로 발견된 이슈는 없습니다(No new issues found). myungjoo-bot 리뷰의 4개 Low 지적 중 3개(커밋 메시지 정정, Javadoc 정확도, |
myungjoo-bot
left a comment
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated re-review of head 7d792b7 (delta since the previously approved 49e23a5; verified directly by the review process, no separate agent this round).
Summary: The tree delta is 2 files, +6/-4: the CustomFilter.Callback.invoke() Javadoc now says a mismatch "fails the invoke and the pipeline stops with an error", the @param data_h doc states that clone must be TRUE for a caller-owned handle, and nns_parse_tensors_data() gains g_return_val_if_fail (*data_h == NULL || clone, FALSE). The three commits were rewritten so the messages are accurate. All three Low items from the previous review are addressed; the fourth (androidTest not in CI) was correctly left as is.
Verified on the head: the new guard is consistent with every caller — nnstreamer-native-customfilter.c:125 passes a non-NULL handle with clone = TRUE; pipeline.c:651 (clone = TRUE), service.c:360 and singleshot.c:237/245 (clone = FALSE) all pass a handle initialised to NULL immediately before the call, so their behaviour is unchanged. The Javadoc wording matches nnstreamer's gst/nnstreamer/tensor_filter/tensor_filter.c on main: invoke_res < 0 posts an invoke-failure application message and returns GST_FLOW_ERROR, while only invoke_res > 0 returns GST_BASE_TRANSFORM_FLOW_DROPPED. The rewritten baadfb5 message now describes the three Android negative cases separately (invalid free / leaked buffer per invoke / unfilled frame reaching the sink), and 394979f adds the empty-list case to the problem statement and documents the clone requirement. All three commits carry Signed-off-by.
CI on 7d792b7: all 10 checks pass, including the four Android ABIs that compile the new guard. The GBS x86_64 unit_test 1 job (102765837671) shows nnstreamer_capi_custom.invoke_output_buffer_p OK (369 ms), 241 tests passed in unittest_capi_inference, every other suite green. merge-base is current main. Approval stands; nothing further requested.
No back-door or suspicious behavior found.
2 participants
Addresses item H4 of #690.
Problem
nns_customfilter_invoke()hands the output handle ofml_pipeline_custom_invoke()tonns_parse_tensors_data(..., clone = TRUE, priv->out_info, &out). That handle is not owned by the JNI layer:ml_pipeline_custom_invoke()pointsout_data->tensors[i].dataat theGstTensorMemoryof tensor_filter (mappedGstMemory, released with_ml_tensors_data_destroy_internal (out_data, FALSE)), and the framework keeps using it after the callback returns.nns_parse_tensors_data()did not distinguish that case from the one where it creates the handle itself, and treated every buffer as its own:CustomFilter.invoke()returns aTensorsDatabuilt from a tensors info other than the registered output info, the capacity differs fromtensors[i].size, so the mapped memory is passed tog_free()— an invalid free of non-GLib memory — and a replacement is allocated. The framework never sees the replacement (the result is lost), and GStreamer later unmaps memory that was already released.data->num_tensorswas overwritten with the Java array length with no bound, so a list longer thanML_TENSOR_SIZE_LIMITwrote past the fixedtensorsarray.TensorsData) passed the parse with nothing copied, so an unfilled frame went downstream.TensorsData.checkByteBuffer()validates a buffer against the info of that object, so an app can build a perfectly validTensorsDatafrom a differentTensorsInfoand still trip all of this.Fix
Split the two ownership cases in
nns_parse_tensors_data():Pipeline.inputData,MLService.request,SingleShot.invoke) — allocate and clone exactly as before.nns_customfilter_invoke()returns -1, which tensor_filter turns intoGST_FLOW_ERROR(the pipeline stops with an error), instead of the heap being corrupted. A caller-owned handle is accepted only withclone == TRUE, since the direct-access path would overwrite the caller's pointers; no caller does that today.The tensor count is bounded by
ML_TENSOR_SIZE_LIMITin both cases.NNStreamer.TENSOR_SIZE_LIMITis the same 256, so no valid Java-side object is rejected by that bound.No change was needed in
nnstreamer-native-customfilter.c: the validation belongs at the single point where the ownership is known.Tests
APITestCustomFiltergains three negative cases that drive aCustomFilterreturning output that does not match the registered output info — a larger buffer (testInvokeInvalidOutputSize_n), an extra tensor (testInvokeInvalidOutputCount_n), and a closed, empty object (testInvokeClosedOutput_n). Before this change the larger buffer passed the mapped memory tog_free(), the extra tensor leaked a buffer on every invoke, and the closed object delivered an unfilled frame to the sink; now the invoke fails for all three and nothing reaches the sink.nnstreamer_capi_custom.invoke_output_buffer_plocks down what the binding relies on: the invoke callback of a custom-easy filter gets an output handle whose tensor count and buffer sizes come from the registered output info, with memory already allocated by tensor_filter, and what it writes there is what the sink receives.Verification
The instrumented tests need a device, so the JNI change itself is not exercised by CI — the Android job only builds it. To verify the fix locally the patched control flow was extracted into a standalone harness and run under ASan/UBSan. With the pre-fix logic the caller-owned buffer is freed inside the parse and then used:
With this change all eight cases (matching output, size mismatch, count mismatch, empty object, new handle with and without a size change, no-clone, over-limit count) pass clean.
🤖 Generated with Claude Code