Coverity static analysis fixes - May 2026#6333
Conversation
dd02952 to
7e4fbaa
Compare
7e4fbaa to
e915521
Compare
|
!build |
|
| Filename | Overview |
|---|---|
| dali/c_api_2/op_test/complex_pipeline_test.cc | Splits reused checkpoint_h into distinct checkpoint1_h/checkpoint2_h locals to eliminate Coverity use-after-RAII-transfer warning; logic is unchanged. |
| dali/c_api_2/pipeline_test_utils.h | Initialises raw_out_h = nullptr in PopOutputs and PopOutputsAsync; prevents RAII wrapper from calling destroy on an uninitialised handle when the non-fatal CHECK_DALI assertion fires. |
| dali/pipeline/operator/operator_factory.h | Takes Registerer::name by value and moves both name and creator into Register; also moves name in the Register<Backend> template overload, eliminating redundant copies on every operator registration. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph complex_pipeline_test.cc
A[daliPipelineGetCheckpoint] -->|writes| B[checkpoint1_h]
B --> C[CheckpointHandle checkpoint1]
D[daliPipelineDeserializeCheckpoint] -->|writes| E[checkpoint2_h]
E --> F[CheckpointHandle checkpoint2]
end
subgraph pipeline_test_utils.h
G["raw_out_h = nullptr"] --> H[daliPipelinePopOutputs]
H -->|success| I[PipelineOutputsHandle wraps valid ptr]
H -->|EXPECT_EQ fails| J[PipelineOutputsHandle wraps nullptr - safe destructor]
end
subgraph operator_factory.h
K["Registerer(std::string name, Creator creator)"] -->|std::move both| L["OperatorRegistry::Register(std::string, Creator, ...)"]
M["Register-Backend-(std::string name, Creator creator)"] -->|std::move both| L
end
Reviews (5): Last reviewed commit: "Address review: drop pipeline.cc change,..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
This PR addresses two Coverity findings in DALI: it updates operator registration to avoid extra copies during static registration, and it refactors a checkpointing test to use RAII for output handles and distinct checkpoint variables.
Changes:
- Changed
Registererto take operator names by value and move both the name and creator intoOperatorRegistry::Register. - Replaced manual output-pop/destroy code in
RunCheckpointingTestwith thePopOutputsRAII helper. - Split a reused checkpoint raw handle into separate
checkpoint1_handcheckpoint2_hvariables in the checkpointing test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dali/pipeline/operator/operator_factory.h |
Updates registration helper argument passing to reduce copies during operator registration. |
dali/c_api_2/op_test/complex_pipeline_test.cc |
Refactors checkpointing test resource management and raw checkpoint handle naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PopOutputs(pipe1); // discard | ||
| CHECK_DALI(daliPipelineRun(pipe1)); | ||
| CHECK_DALI(daliPipelinePopOutputs(pipe1, &out1_h)); | ||
| CHECK_DALI(daliPipelineOutputsDestroy(out1_h)); | ||
| PopOutputs(pipe1); // discard | ||
| CHECK_DALI(daliPipelineRun(pipe1)); | ||
| CHECK_DALI(daliPipelinePopOutputs(pipe1, &out1_h)); | ||
| CHECK_DALI(daliPipelineOutputsDestroy(out1_h)); | ||
| PopOutputs(pipe1); // discard |
There was a problem hiding this comment.
- The test was meant to be run on raw C API. It's a C API test, after all.
- The comment about daliPipelinePopOutputs is correct - the function is broken and needs fixing.
|
CI MESSAGE: [50302622]: BUILD STARTED |
79eb5d9 to
30f33cd
Compare
…stry, broken PopOutputs contract complex_pipeline_test.cc: split the reused checkpoint_h into distinct checkpoint1_h / checkpoint2_h locals so there is no apparent reuse of a handle after a RAII transfer, addressing the Coverity-flagged use-after-free in RunCheckpointingTest. The test keeps using the raw C API for output handles (it is intentionally a C API test). pipeline.cc: in daliPipelinePopOutputs and daliPipelinePopOutputsAsync, set *out = nullptr after CHECK_OUTPUT and before any operation that may throw, so the failure-path contract is well-defined. Without this, if ToPointer(pipeline) or pipe->PopOutputs() throws, DALI_EPILOG returns an error code without ever writing *out, leaving the caller's output variable in an indeterminate state. operator_factory.h: take Registerer::name by value and std::move both name and creator into OperatorRegistry::Register, eliminating two copies on each operator registration. Also std::move(name) in the Register<Backend> template overload so the name reaches the inner Register without an extra copy. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
30f33cd to
95f6d4f
Compare
| *out = nullptr; | ||
| auto pipe = ToPointer(pipeline); |
There was a problem hiding this comment.
This is against the convention. If the function fails, it should not modify the output variable.
FWIW, CUDA runtime APIs behave the same way - if the arguments are invalid, no output is returned:
#include <cuda_runtime.h>
#include <stdio.h>
int main() {
cudaStream_t s = (cudaStream_t)0x12345678;
cudaError_t err = cudaStreamCreateWithFlags(&s, 0x1234568);
printf("%i %p\n", err, s);
return 0;
}Output:
1 0x12345678
| *out = nullptr; | ||
| auto pipe = ToPointer(pipeline); |
|
CI MESSAGE: [50302622]: BUILD PASSED |
Per review feedback: writing *out = nullptr inside daliPipelinePopOutputs and daliPipelinePopOutputsAsync violates the C API convention shared with the CUDA runtime - on a non-success return, output parameters are not modified (only DALI_NO_DATA is allowed to write nullptr to *out, and neither of these functions returns that status). Revert the pipeline.cc change in full so the surface stays consistent with the rest of the file. The actual issue Copilot pointed at lives on the caller side: the PopOutputs / PopOutputsAsync helpers in pipeline_test_utils.h declared daliPipelineOutputs_h raw_out_h; uninitialized, and CHECK_DALI in this codebase is EXPECT_EQ (non-fatal). On a failed call the helpers wrapped an indeterminate handle in PipelineOutputsHandle and the destructor would call daliPipelineOutputsDestroy on garbage. Initialize raw_out_h = nullptr in both helpers so the destructor sees nullptr on the failure path and the original EXPECT_EQ failure stays visible. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
|
!build |
|
CI MESSAGE: [50328210]: BUILD STARTED |
|
CI MESSAGE: [50328210]: BUILD PASSED |
Summary
Three changes:
complex_pipeline_test.cc— Coverity-flagged use-after-free inRunCheckpointingTest. Splitcheckpoint_hinto distinctcheckpoint1_h/checkpoint2_hlocals so there is no apparent reuse of a handle after a RAII transfer. The test keeps using the raw C API for output handles (it is intentionally a C API test).pipeline_test_utils.h— initializedaliPipelineOutputs_h raw_out_h = nullptrin thePopOutputsandPopOutputsAsynctest helpers.CHECK_DALIhere isEXPECT_EQ(non-fatal), so on a failing call the helpers were wrapping an uninitialized handle inPipelineOutputsHandleand the destructor would calldaliPipelineOutputsDestroyon garbage. With this init the destructor sees nullptr on the failure path and the originalEXPECT_EQfailure stays visible. The C API itself is unchanged: per the convention shared with the CUDA runtime, output parameters are not modified on non-success returns (onlyDALI_NO_DATAwrites nullptr, which these functions do not return), so initialization is the caller's responsibility.operator_factory.h— Coverity-flagged copy-when-could-move inRegisterer. TakeRegisterer::nameby value andstd::movebothnameandcreatorintoOperatorRegistry::Register, eliminating two copies on each operator registration. Alsostd::move(name)in theRegister<Backend>template overload so the name reaches the innerRegisterwithout an extra copy.The remaining Coverity findings from the same snapshot (an unchecked-return-value in
FileLabelLoaderBaseand an uncaught-exception in vendoredthird_party/pybind11) are intentionally not addressed here.Test plan
CAPI2_SerializedPipelineTest.Checkpointing/CAPI2_PipelineBuilderTest.CheckpointingRunCheckpointingTestor the copy-when-could-move inRegisterer