| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Coverity CID 414657 (REVERSE_INULL): a rejected item-acquire path in `dict_item_add_or_reset_value_and_acquire()` left `item` non-NULL, so the `do/while` loop exited instead of retrying after stale view-entry cleanup. Clear `item` before retrying and cover the stale-view replacement path in the existing dictionary unittest.
Release the acquired `view_item2` on the stale/deleted failure branch of the view-replacement regression test, matching the release on the success branch so the test does not leak an acquired reference.
Coverity CID 405093 (MISSING_LOCK): ebpf_module.enabled was sampled from stats and shutdown paths while module exit code updated the same plain enum under a different synchronization regime. Convert the live cross-thread reads and writes to atomic helpers so those state checks stay defined without changing the existing lock layout.
Coverity CID 405089 (SLEEP): Function-triggered eBPF socket restarts held ebpf_exit_cleanup while nd_thread_create could wait and retry. Gate the new thread until state is published, so creation happens outside the cleanup mutex without racing shutdown.
nd_journal_directory_scan_recursively() used depth++ (post-increment) when recursing into subdirectories, which passes the caller's current depth to the recursive call and then increments the caller's local counter across sibling iterations. Effect: the 2nd, 3rd, ... sibling subdirectories of the same parent get inflated depths and prematurely hit VAR_LOG_JOURNAL_MAX_DEPTH, silently truncating legitimate scans. Use depth + 1 instead so every recursion starts exactly one level deeper than the current frame.
When dictionary_set() returns NULL the recursive scan still closes the current directory and bails out, but an operator looking at truncated journal discovery had no way to know why. Log the failure before returning so the condition is diagnosable.
|
Sorry, something went wrong.
There was a problem hiding this comment.
2 issues found across 24 files
Confidence score: 3/5
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/collectors/ebpf.plugin/libbpf_api/ebpf_library.c">
<violation number="1" location="src/collectors/ebpf.plugin/libbpf_api/ebpf_library.c:530">
P1: This check negates an enum status value, which inverts the logic because RUNNING is 0. Compare explicit states instead so the socket chart is enabled only when the module is not running.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpf_hardirq.c">
<violation number="1" location="src/collectors/ebpf.plugin/ebpf_hardirq.c:227">
P1: This cleanup branch is unreachable because `hardirq_cleanup()` returns at the top of the function.</violation>
</file>
sequenceDiagram
participant P as eBPF Plugin Main
participant PT as Parked Startup Thread
participant MT as Module Collector Thread
participant S as Shared Module State
participant J as Journal Service
participant D as Dictionary View
Note over P,S: NEW: eBPF Thread Lifecycle (Race Prevention)
P->>PT: Create parked thread with context
PT->>PT: Loop: tinysleep() while !ready (Atomic)
P->>P: Lock(ebpf_exit_cleanup)
alt Plugin not stopping
P->>S: NEW: ebpf_module_enabled_set(FUNCTION_RUNNING)
P->>PT: Set run = true, ready = true
else Shutdown in progress
P->>PT: Set run = false, ready = true
end
P->>P: Unlock
PT->>PT: Wake up on ready=true
alt run is true
PT->>MT: Start module routine (e.g., ebpf_process_thread)
else run is false
PT->>PT: Self-terminate
end
Note over P,MT: CHANGED: eBPF Status & Monitoring
P->>S: NEW: ebpf_module_enabled_get() (Atomic Relaxed)
Note right of S: Avoids lock contention during stats collection
Note over MT,S: eBPF Shutdown Path
MT->>MT: Traps signal/exit
MT->>P: Lock(ebpf_exit_cleanup)
MT->>S: NEW: ebpf_module_enabled_set(STOPPED)
MT->>P: Unlock
Note over J: CHANGED: Journal Directory Recursion
J->>J: Scan directory
alt Entry is Directory
J->>J: CHANGED: Recurse with depth + 1
Note right of J: Prevents truncated scans/inflated depth
end
alt Visited Dictionary Full
J-->>J: NEW: Log error & stop recursion
end
Note over D: Dictionary Stale Entry Logic
D->>D: Attempt view entry replacement
alt Stale/Deleted item found
D->>D: NEW: Clear rejected pointer and retry
Note right of D: Prevents crashes on stale view lookup
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Sorry, something went wrong.
There was a problem hiding this comment.
This PR addresses several memory-safety, race, and correctness issues identified during a Coverity audit, focusing on safer dictionary view replacement behavior, reliable systemd journal directory traversal, and more thread-safe eBPF module state handling during runtime/shutdown.
Changes:
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file| File | Description |
|---|---|
| src/libnetdata/dictionary/dictionary-unittest.c | Adds regression coverage for replacing a stale view item after master deletion. |
| src/libnetdata/dictionary/dictionary-item.h | Fixes retry-loop correctness by clearing item after a rejected acquire. |
| src/collectors/systemd-journal.plugin/systemd-journal-files.c | Fixes recursion depth and improves error handling when visited-dir tracking fails. |
| src/collectors/ebpf.plugin/libbpf_api/ebpf_library.c | Replaces direct em->enabled writes/reads with atomic helper accessors. |
| src/collectors/ebpf.plugin/libbpf_api/ebpf.c | Uses atomic helper accessor for module state reads in stats path. |
| src/collectors/ebpf.plugin/ebpf_vfs.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_sync.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_swap.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_softirq.c | Uses atomic helper accessors for module state checks/updates during cleanup. |
| src/collectors/ebpf.plugin/ebpf_socket.c | Uses atomic helper accessors for module state updates and function exit checks. |
| src/collectors/ebpf.plugin/ebpf_shm.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_process.c | Uses atomic helper accessors for module state changes on invalid state / failures / exit. |
| src/collectors/ebpf.plugin/ebpf_oomkill.c | Uses atomic helper accessors for module state checks/updates during cleanup and disable logging. |
| src/collectors/ebpf.plugin/ebpf_mount.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_mdflush.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_hardirq.c | Uses atomic helper accessors for module state checks/updates during cleanup. |
| src/collectors/ebpf.plugin/ebpf_functions.c | Introduces “parked” thread start to publish state under ebpf_exit_cleanup before running routines. |
| src/collectors/ebpf.plugin/ebpf_filesystem.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_fd.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_disk.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_dcstat.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf_cachestat.c | Uses atomic helper accessors for module state checks/updates during exit. |
| src/collectors/ebpf.plugin/ebpf.h | Adds atomic ebpf_module_enabled_get/set() helpers and updates validity checks to use them. |
| src/collectors/ebpf.plugin/ebpf.c | Uses atomic helper accessors for module state checks in shutdown/stats/startup paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Everything ran as expected. LGTM!
Sorry, something went wrong.
…rt 5) (#22279) * dictionary: retry view inserts after stale entry cleanup Coverity CID 414657 (REVERSE_INULL): a rejected item-acquire path in `dict_item_add_or_reset_value_and_acquire()` left `item` non-NULL, so the `do/while` loop exited instead of retrying after stale view-entry cleanup. Clear `item` before retrying and cover the stale-view replacement path in the existing dictionary unittest. * dictionary: release view item on stale replacement branch in unittest Release the acquired `view_item2` on the stale/deleted failure branch of the view-replacement regression test, matching the release on the success branch so the test does not leak an acquired reference. * ebpf.plugin: make module state accesses atomic Coverity CID 405093 (MISSING_LOCK): ebpf_module.enabled was sampled from stats and shutdown paths while module exit code updated the same plain enum under a different synchronization regime. Convert the live cross-thread reads and writes to atomic helpers so those state checks stay defined without changing the existing lock layout. * ebpf: start function threads outside cleanup lock Coverity CID 405089 (SLEEP): Function-triggered eBPF socket restarts held ebpf_exit_cleanup while nd_thread_create could wait and retry. Gate the new thread until state is published, so creation happens outside the cleanup mutex without racing shutdown. * systemd-journal: pass depth+1 to recursive directory scan nd_journal_directory_scan_recursively() used depth++ (post-increment) when recursing into subdirectories, which passes the caller's current depth to the recursive call and then increments the caller's local counter across sibling iterations. Effect: the 2nd, 3rd, ... sibling subdirectories of the same parent get inflated depths and prematurely hit VAR_LOG_JOURNAL_MAX_DEPTH, silently truncating legitimate scans. Use depth + 1 instead so every recursion starts exactly one level deeper than the current frame. * systemd-journal: log visited directory tracking failures When dictionary_set() returns NULL the recursive scan still closes the current directory and bails out, but an operator looking at truncated journal discovery had no way to know why. Log the failure before returning so the condition is diagnosable. --------- Co-authored-by: Costa Tsaousis <costa@netdata.cloud> (cherry picked from commit 42097e1)
| Back | FazBrowse Home | New Git URL |
Summary
Summary by cubic
Fixes race conditions and correctness issues found by Coverity: makes eBPF module state updates thread-safe, prevents thread-start races during shutdown, fixes journal directory scan depth, and resolves a dictionary stale-view insert bug with tests. Improves shutdown reliability, avoids truncated journal discovery, and removes intermittent crashes.
Written for commit 95c0002. Summary will update on new commits.