Conversation
greenc-FNAL
changed the title
clang-tidy safety/security reports
|
@phlexbot clang-fix |
|
Automatic clang-format fixes pushed (commit 19ef651). |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #375 +/- ##
=======================================
Coverage 86.45% 86.45%
=======================================
Files 119 119
Lines 2399 2400 +1
Branches 387 387
=======================================
+ Hits 2074 2075 +1
Misses 207 207
Partials 118 118
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
knoepfel
left a comment
knoepfel
left a comment
There was a problem hiding this comment.
@greenc-FNAL, I agree with almost all of these. I think the use of std::ctime(...) in test/hierarchical_nodes.cpp should probably be replaced with modern C++ usage.
- Checks run:
- `clang-analyzer-security-*`
- `bugprone-unsafe-functions`
- `concurrency-mt-unsafe`
```console
[310/437] Building CXX object phlex/app/CMakeFiles/run_phlex.dir/load_module.cpp.o
/phlex-src/phlex/app/load_module.cpp:32:37: warning: function is not thread safe [concurrency-mt-unsafe]
32 | char const* plugin_path_ptr = std::getenv("PHLEX_PLUGIN_PATH");
| ^
[332/437] Building CXX object test/CMakeFiles/hierarchical_nodes.dir/hierarchical_nodes.cpp.o
/phlex-src/test/hierarchical_nodes.cpp:70:26: warning: function is not thread safe [concurrency-mt-unsafe]
70 | std::strncpy(buffer, std::ctime(&tm), 26);
| ^
[344/437] Building CXX object test/form/CMakeFiles/WriteVector.dir/toy_tracker.cpp.o
/phlex-src/test/form/toy_tracker.cpp:22:15: warning: function is not thread safe [concurrency-mt-unsafe]
22 | int rand1 = rand() % 32768;
| ^
/phlex-src/test/form/toy_tracker.cpp:23:15: warning: function is not thread safe [concurrency-mt-unsafe]
23 | int rand2 = rand() % 32768;
| ^
[364/437] Building CXX object test/form/CMakeFiles/WriteVector.dir/writer.cpp.o
/phlex-src/test/form/writer.cpp:23:15: warning: function is not thread safe [concurrency-mt-unsafe]
23 | int rand1 = rand() % 32768;
| ^
/phlex-src/test/form/writer.cpp:24:15: warning: function is not thread safe [concurrency-mt-unsafe]
24 | int rand2 = rand() % 32768;
| ^
/phlex-src/test/form/writer.cpp:27:17: warning: function is not thread safe [concurrency-mt-unsafe]
27 | int rand1 = rand() % 32768;
| ^
/phlex-src/test/form/writer.cpp:28:17: warning: function is not thread safe [concurrency-mt-unsafe]
28 | int rand2 = rand() % 32768;
| ^
```
No automatic fixes
- Surgical suppression of each check after verification.
- Remove use of thread-unsafe C-style time structures and functions.
greenc-FNAL
force-pushed
the
maintenance/clang-tidy-security-safety-1
branch
from
19ef651 to
9dd8b2c
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts code to address clang-tidy safety/security warnings (notably concurrency-mt-unsafe) by replacing non-thread-safe time formatting, adding targeted NOLINT suppressions in test code, and simplifying/removing build-time clang-tidy CMake integration.
Changes:
- Replace
std::ctimeusage intest/hierarchical_nodes.cppwithstd::chrono+fmtformatting to avoid non-thread-safe time APIs. - Add
NOLINT(concurrency-mt-unsafe)annotations forrand()/getenv()call sites with “single-threaded” rationale. - Remove
ENABLE_CLANG_TIDYoption and associated clang-tidy configuration block from the rootCMakeLists.txt.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/hierarchical_nodes.cpp |
Switches from ctime to chrono + fmt time formatting and simplifies log formatting. |
test/form/writer.cpp |
Suppresses rand() thread-safety warnings in single-threaded test code. |
test/form/toy_tracker.cpp |
Suppresses rand() thread-safety warnings in test helper code. |
phlex/app/load_module.cpp |
Suppresses getenv() thread-safety warning with a “single-threaded graph construction” note. |
CMakeLists.txt |
Removes CMake option-based clang-tidy integration. |
knoepfel
left a comment
knoepfel
left a comment
There was a problem hiding this comment.
Changes look good, @greenc-FNAL. I think some of Copilot's comments are worth taking into account. Please resolve any comments you think need no further action.
- Per #375 (review) - Add missing headers - Update `CLANG_TIDY_CONFIGRATION.md`, satisfying #375 (comment)
bugprone-unsafe-functionsclang-analyzer-core.BitwiseShiftclang-analyzer-core.CallAndMessageclang-analyzer-core.CallAndMessageModelingclang-analyzer-core.DereferenceModelingclang-analyzer-core.DivideZeroclang-analyzer-core.DynamicTypePropagationclang-analyzer-core.FixedAddressDereferenceclang-analyzer-core.NonNullParamCheckerclang-analyzer-core.NonnilStringConstantsclang-analyzer-core.NullDereferenceclang-analyzer-core.StackAddrEscapeBaseclang-analyzer-core.StackAddressEscapeclang-analyzer-core.UndefinedBinaryOperatorResultclang-analyzer-core.VLASizeclang-analyzer-core.builtin.AssumeModelingclang-analyzer-core.builtin.BuiltinFunctionsclang-analyzer-core.builtin.NoReturnFunctionsclang-analyzer-core.uninitialized.ArraySubscriptclang-analyzer-core.uninitialized.Assignclang-analyzer-core.uninitialized.Branchclang-analyzer-core.uninitialized.CapturedBlockVariableclang-analyzer-core.uninitialized.NewArraySizeclang-analyzer-core.uninitialized.UndefReturnclang-analyzer-security.ArrayBoundclang-analyzer-security.FloatLoopCounterclang-analyzer-security.MmapWriteExecclang-analyzer-security.PointerSubclang-analyzer-security.PutenvStackArrayclang-analyzer-security.SetgidSetuidOrderclang-analyzer-security.cert.env.InvalidPtrclang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandlingclang-analyzer-security.insecureAPI.SecuritySyntaxCheckerclang-analyzer-security.insecureAPI.UncheckedReturnclang-analyzer-security.insecureAPI.bcmpclang-analyzer-security.insecureAPI.bcopyclang-analyzer-security.insecureAPI.bzeroclang-analyzer-security.insecureAPI.decodeValueOfObjCTypeclang-analyzer-security.insecureAPI.getpwclang-analyzer-security.insecureAPI.getsclang-analyzer-security.insecureAPI.mkstempclang-analyzer-security.insecureAPI.mktempclang-analyzer-security.insecureAPI.randclang-analyzer-security.insecureAPI.strcpyclang-analyzer-security.insecureAPI.vforkconcurrency-mt-unsafeInitial report:
No automatic fixes available.