Coverity Fixes #12821
Coverity Fixes #12821
Conversation
The ts::bravo::shared_lock properly holds the mutex but Coverity doesn't recognize this custom lock class. Add suppression comment.
Wrap potentially throwing diagnostic code (Dbg, ink_assert) in try-catch to ensure destructor never throws. Memory cleanup continues regardless.
… test The test intentionally uses copy assignment (not move) to verify copy assignment operator behavior. Add suppression comment.
Initialize entrylist to nullptr before passing to scandir to satisfy static analysis, even though scandir allocates the memory.
PSUdaemon
force-pushed
the
coverity
branch
2 times, most recently
from
9a88c3d to
f23df19
Compare
|
[approve ci osx] |
|
The elimination of lock guards for string metrics (with the finalize thing) seems weird. @cmcfarlen Can't we create string metrics after startup / initialization? Maybe we can't? On the try / catch, I'm not convinced we should do that, but rather, understand why Coverity don't like those destructors / release mechanism, and/or why they can raise exceptions. |
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Looks great. Thanks for the fixes.
I walked through all the commits (thanks for keeping them separate) and they all seem make sense to me, but I have a question about the crashlog one.
| int | ||
| main(int /* argc ATS_UNUSED */, const char **argv) | ||
| { | ||
| // coverity[fun_call_w_exception] - ats_as_c_str checks optional before .value(), | ||
| // and callers guard with if(opt) before calling ats_as_c_str(opt) | ||
| FILE *fp; | ||
| char *logname; |
There was a problem hiding this comment.
Can you explain how this suppression is working? I would have guessed tha the coverity exemption comment would have to be before the function call itself rather than before the FILE declaration. I might be missing something though.
There was a problem hiding this comment.
My understanding is that it can be at function scope as well, but that's probably too broad for this instance and I have moved it. Thanks for pointing this out.
It appears that plugin metric creation goes through |
|
[approve ci osx] |
|
[approve ci autest] |
Yeah, it's possible that some external plugin (or someone's custom code that isn't open sourced) makes string metrics after startup. I don't think we should restrict it to static initializers. |
Add proper mutex protection to StaticString: - Add mutex locks to _createString() and lookup() - Add for_each() method for thread-safe iteration - Remove begin()/end() which exposed non-thread-safe iterators - Update RecCore.cc to use for_each()
…structor Add suppression comment. The destroy() method only frees memory and does ref counting - it cannot throw.
The visitor intentionally moves from the variant alternative to consume it. The caller should not use the moved-from value after the visitor returns.
The function calls Lexicon::operator[] which uses std::visit internally and can throw std::domain_error for unknown unit names. Remove noexcept to allow the exception to propagate.
Wrap m_remap->release() in try-catch since it can allocate (new_Deleter). Add suppression comments for other calls that cannot throw.
Replace strcpy with memcpy using explicit length from string_view. This satisfies static analysis even though the assert validates the size. Also explicitly null-terminate the destination.
Add REQUIRE(result->hostname \!= nullptr) before strcmp calls to ensure the pointer is valid, preventing potential null dereference.
Make the Lexicon static to avoid capturing 360 bytes by value in the lambda. The lambda now captures nothing and references the static lexicon directly.
Add suppression comment for false positive. The heap is freed via hdr.destroy() -> HdrHeapSDKHandle::destroy() -> m_heap->destroy().
Use std::move when assigning opt_val to _strategy since opt_val is not used again after this assignment in the strategy branch.
Check the return value of TSCacheUrlSet and return an error Errata if the call fails.
Add Coverity suppression comment for false positive. The len parameter is validated positive by REQUIRE(len > 0) before the memcmp call, but Coverity doesn't recognize that REQUIRE throws on failure.
Add Coverity suppression for false positive. The plugin pointer temporarily escapes to pluginThreadContext in doneInstance() but is properly reset via resetPluginContext() before the method returns.
Add Coverity suppression for false positive. The std::optional access via ats_as_c_str() is properly guarded by checking the optional has a value before calling the function.
Add Coverity suppression. Functions called from main() may throw exceptions, but this is a test program where uncaught exceptions will terminate with a stack trace, which is acceptable behavior.
The RECD_COUNTER case in plugin_expand() had an inverted condition: 'if (count_val)' instead of 'if (!count_val)'. This caused the code to call .value() on an empty optional when the counter record was not found, throwing std::bad_optional_access. This matches the pattern used in the RECD_FLOAT and RECD_INT cases.
4 participants
Fixes 19 Coverity warnings. Also found and fixed a real bug - inverted condition in Plugin.cc that would crash on counter record lookups.
Most fixes are straightforward: uninitialized variables, unchecked return values, missing null checks, resource leaks in tests. A few are false positive suppressions with comments explaining why.
Destructor exception safety fixes for
HttpSMandHttpTransact::State.One commit per issue, CID in each commit message.