FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix memory-safety and correctness bugs surfaced by Coverity audit (part 6) by stelfrag · Pull Request #22281 · netdata/netdata · GitHub

Fix memory-safety and correctness bugs surfaced by Coverity audit (part 6) - #22281

Merged
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_part6
May 3, 2026
Merged

Fix memory-safety and correctness bugs surfaced by Coverity audit (part 6)#22281
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_part6

Conversation

stelfrag commented Apr 25, 2026
edited by cubic-dev-ai Bot
Loading

Copy link
Copy Markdown
Collaborator
Summary

Summary by cubic

Fixes memory-safety and concurrency bugs found by Coverity by serializing chart alert deletion and preventing lost worker wakeups. Improves correctness during alert cleanup and queued job processing.

  • Bug Fixes
    • Health (alert cleanup): Serialize deletion of a chart’s alerts by taking the host alert dictionary write lock while walking the chart list, preventing races with the health thread (CID 380968).
    • Functions event loop: Scan the queue and wait under the same worker_mutex so new-job signals can’t be missed and no requests are stranded (CID 439969).

Written for commit 5c2685c. Summary will update on new commits. Review in cubic

cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No issues found across 26 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.
Architecture diagram
sequenceDiagram
    participant Main as Plugin Main
    participant Trampoline as NEW: Start Trampoline
    participant Mod as eBPF Module
    participant Thread as Module Worker Thread
    participant Stats as Stats / Shutdown Path

    Note over Main, Mod: Thread Startup Flow
    Main->>Trampoline: nd_thread_create(ebpf_function_thread_start)
    activate Trampoline
    Trampoline->>Trampoline: NEW: Park thread (spin on atomic 'ready')
    
    Main->>Main: netdata_mutex_lock(&ebpf_exit_cleanup)
    Main->>Mod: NEW: ebpf_module_enabled_set(RUNNING) [Atomic Store]
    Main->>Main: Set em->lifetime
    Main->>Trampoline: NEW: Set context->ready = true [Atomic Release]
    Main->>Main: netdata_mutex_unlock(&ebpf_exit_cleanup)

    Trampoline->>Thread: Execute start_routine(em)
    deactivate Trampoline
    activate Thread

    Note over Stats, Mod: Asynchronous Monitoring / Shutdown
    Stats->>Mod: NEW: ebpf_module_enabled_get() [Atomic Load]
    Note right of Mod: CHANGED: No longer requires global cleanup lock<br/>to check if module is active.
    
    alt Shutdown Signal
        Stats->>Thread: nd_thread_signal_cancel()
        Thread->>Thread: Perform cleanup
        Thread->>Mod: NEW: ebpf_module_enabled_set(STOPPED) [Atomic Store]
        deactivate Thread
    end

    Note over Main, Thread: Functions Event Loop Correction
    participant Queue as Functions Job Queue
    participant Worker as Event Loop Worker
    participant Mutex as Worker Mutex

    loop Event Loop
        Worker->>Mutex: Lock
        Worker->>Queue: NEW: Scan for jobs AND Wait under same lock
        alt Job Found
            Queue-->>Worker: Return Job
            Worker->>Worker: Mark j->running = true
            Worker->>Mutex: Unlock
            Worker->>Worker: Execute Callback
        else No Job
            Worker->>Mutex: netdata_cond_wait (atomically release/wait)
            Note right of Worker: Prevents missed wakeups between scan and sleep.
        end
    end
Loading

ktsaou added 2 commits April 25, 2026 20:34
Coverity CID 380968 (MISSING_LOCK): `rrdcalc_unlink_and_delete_all_rrdset_alerts()`
read alert values before taking the host alert dictionary write traversal lock,
so the health thread could update `rc->value` concurrently. Delete matching
alerts under `foreach_rrdcalc_in_rrdhost_write()` to keep the unlink snapshot
synchronized with health evaluation.
Coverity CID 439969 (BAD_CHECK_OF_WAIT_COND): the worker loop could miss a
new-job signal after failing to acquire runnable work and then sleep even
though a request was already queued. Scan for runnable jobs and wait under the
same mutex so queued requests cannot be stranded behind a lost wakeup.

stelfrag commented Apr 25, 2026
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai write summary in the PR description

cubic-dev-ai Bot commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai write summary

@stelfrag This PR addresses memory-safety and concurrency bugs identified by a Coverity audit, focusing on alert cleanup and worker thread synchronization.

Key Changes

  • Health (Alert Cleanup): In src/health/rrdcalc.c, the cleanup of alerts linked to a specific chart (rrdset) is now serialized. By using foreach_rrdcalc_in_rrdhost_write, the removal process is performed under the host-level alert dictionary write lock. This prevents race conditions with the health thread that could otherwise update alert values concurrently during the cleanup (fixes Coverity CID 380968).
  • Functions Event Loop (Worker Wakeups): In src/libnetdata/functions_evloop/functions_evloop.c, the worker loop was refactored to prevent "lost wakeups." The job scan and the condition wait now happen under the same worker_mutex. This ensures that if a new job is queued, the signal cannot be missed by a thread that has decided to sleep but hasn't yet blocked (fixes Coverity CID 439969).

Related Resources

stelfrag marked this pull request as ready for review April 25, 2026 18:18
stelfrag requested a review from thiagoftsm as a code owner April 25, 2026 18:18
Copilot AI review requested due to automatic review settings April 25, 2026 18:18
stelfrag requested a review from vkalintiris as a code owner April 25, 2026 18:18
stelfrag marked this pull request as draft April 25, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

This PR addresses Coverity-reported correctness and concurrency issues by tightening synchronization around alert cleanup and worker-thread wakeups in the functions event loop.

Changes:

  • Serialize removal of all RRDCALC alerts for a chart under the host’s RRDCALC dictionary write traversal to avoid races with health-thread updates.
  • Refactor functions event-loop worker wakeups so queue scanning and condition-variable waits happen under the same mutex to prevent lost wakeups.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/libnetdata/functions_evloop/functions_evloop.c Refactors worker loop locking/cond-wait behavior to avoid missed wakeups when jobs are queued.
src/health/rrdcalc.c Reworks chart-alert cleanup to run under host-level RRDCALC write traversal, avoiding concurrent update races.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

stelfrag requested a review from Copilot April 30, 2026 12:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/health/rrdcalc.c
stelfrag marked this pull request as ready for review May 2, 2026 09:25

stelfrag commented May 2, 2026

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

cubic-dev-ai Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@stelfrag I have started the AI code review. It will take a few minutes to complete.

thiagoftsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I check different charts from different plugins running this PR, and everything is working as expected. LGTM!

stelfrag merged commit 3a902fd into netdata:master May 3, 2026
169 of 170 checks passed
stelfrag deleted the cov_fix_part6 branch May 3, 2026 09:00
stelfrag mentioned this pull request Jun 22, 2026
Ferroin pushed a commit that referenced this pull request Jul 15, 2026
…rt 6) (#22281)

* health: serialize rrdset alert cleanup

Coverity CID 380968 (MISSING_LOCK): `rrdcalc_unlink_and_delete_all_rrdset_alerts()`
read alert values before taking the host alert dictionary write traversal lock,
so the health thread could update `rc->value` concurrently. Delete matching
alerts under `foreach_rrdcalc_in_rrdhost_write()` to keep the unlink snapshot
synchronized with health evaluation.

* functions_evloop: fix missed worker wakeup for queued jobs

Coverity CID 439969 (BAD_CHECK_OF_WAIT_COND): the worker loop could miss a
new-job signal after failing to acquire runnable work and then sleep even
though a request was already queued. Scan for runnable jobs and wait under the
same mutex so queued requests cannot be stranded behind a lost wakeup.

* Address review comments / re-work lock strategy when deleting chart alerts

---------

Co-authored-by: Costa Tsaousis <costa@netdata.cloud>
(cherry picked from commit 3a902fd)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL