Skip to content

Netdata fixes part 33 - #22973

Merged
stelfrag merged 4 commits into
netdata:masterfrom
stelfrag:netdata-fixes-33
Jul 7, 2026
Merged

Netdata fixes part 33#22973
stelfrag merged 4 commits into
netdata:masterfrom
stelfrag:netdata-fixes-33

Conversation

@stelfrag

@stelfrag stelfrag commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator
Summary

Summary by cubic

Hardens the circular buffer to prevent infinite growth loops, integer overflow, and out-of-range write positions, with no change for valid callers.

  • Bug Fixes
    • Growth: seed zero-size buffers at 1 byte and cap before doubling to avoid stuck loops and overflow.
    • Capacity checks: reject impossible add/reserve requests upfront; use subtraction-based bounds (no wrapped d_len + len or write + size).
    • Commit safety: validate inputs, ensure commit fits the remaining ring capacity, and update the write index with wrap-safe arithmetic; ignore overlarge/invalid commits.

Written for commit 0a4f672. Summary will update on new commits.

Review in cubic

ktsaou added 4 commits July 5, 2026 10:10
cbuffer_realloc_unsafe() doubled the current buffer size to compute the next allocation. When a buffer was created with initial size 0 and a positive maximum size, zero doubled to zero, the helper returned success without increasing buf->size, and the add/reserve loops could repeat forever.

Seed the growth calculation with one byte when the current size is zero, then keep the existing max-size cap. All non-zero buffers keep the same doubling behavior, and max_size 0 still fails before the growth calculation.

(cherry picked from commit 82704a2)
Circular-buffer growth and write paths performed size_t addition before capacity checks. Doubling buf->size could wrap before max_size clamping, and d_len + len, size + len, and buf->write + size could wrap before deciding whether a write or reserve fits.

Saturate growth before multiplying, reject impossible requests before the growth loop, and rewrite capacity and write-position updates in subtraction form. Valid buffer behavior, return values, and growth semantics are unchanged; only wrapped arithmetic paths now fail or saturate safely.

(cherry picked from commit 34158ed)
Issue: cbuffer_commit_reserved_unsafe() advanced the write cursor with a single wrap adjustment. Current WebSocket callers reserve and commit matching bounded sizes, so the issue is potential in the reviewed call graph, but a contract-violating helper caller could commit more than one ring length and leave buf->write outside the backing allocation.

Current vs potential: valid production callers are not known to exercise an over-sized commit today. The helper itself currently accepts arbitrary size_t input, so misuse inside future callers or error paths could preserve an out-of-range cursor after one invalid call.

Solution: return before any arithmetic when the buffer pointer, data pointer, commit length, or ring size is unusable, reduce the committed size modulo that ring size, and then apply the existing wrap logic. The null and zero-size checks are written as explicit control-flow guards before loading buf->size and before the modulo.

Functional impact: no change for valid callers; invalid over-commit sizes now keep the cursor in range instead of corrupting the ring invariant. Null buffers, missing data, zero-size buffers, and zero-size commits remain no-ops.

Performance impact: one size_t modulo per reserved commit, negligible beside current WebSocket socket I/O and frame processing paths. Splitting the exceptional guards into explicit branches has no meaningful cost on the valid path.

Security impact: prevents a reusable circular-buffer helper from preserving an out-of-range write cursor after invalid commit sizes.

Validation: git diff --check passed for src/libnetdata/circular_buffer/circular_buffer.c. A debugfs-enabled CMake configure passed after initializing submodules. cmake --build for debugfs.plugin passed and recompiled src/libnetdata/circular_buffer/circular_buffer.c into libnetdata.

Reviewer gate: required local semantic review gate passed for the original finding, and follow-up static-analysis feedback was verified against the source before this fixup.

(cherry picked from commit 8d6abfe)
cbuffer_commit_reserved_unsafe() documents that callers should commit no more than the size returned by cbuffer_reserve_unsafe(), but the helper did not enforce the circular-buffer capacity invariant. The previous modulo hardening kept the write index inside the allocation, but a contract-violating overlarge commit could still be reduced to a smaller commit and make the buffer state represent bytes that were never reserved.

Current direct WebSocket callers reserve and commit bounded sizes, so no reviewed production caller triggers this today. The reusable helper still accepted arbitrary size_t input, so future or error-path misuse could corrupt the logical ring state.

Compute the current used size and return without advancing the cursor when the buffer is already in an impossible full/corrupt state or when the requested commit would fill or overrun the representable ring capacity. Remove the modulo because accepted commits are already smaller than the ring size.

Functional impact: valid reserve/commit callers are unchanged. Invalid overlarge commits now leave the buffer unchanged instead of silently committing a modulo-reduced byte count.

Performance impact: one used-size calculation and one unlikely branch per reserved commit, negligible beside current WebSocket socket I/O and frame processing.

Security impact: hardens a reusable circular-buffer helper against state corruption from internal misuse and preserves the one-free-byte invariant used to distinguish empty from full.

Validation: git diff --check; built the circular-buffer object, WebSocket receive/send caller objects, libnetdata, and debugfs.plugin.

Review: required independent source review approved the exact diff; one non-blocking adjacent maintainability observation was recorded separately.
(cherry picked from commit ec46047)
@sonarqubecloud

sonarqubecloud Bot commented Jul 5, 2026

Copy link
Copy Markdown

@cubic-dev-ai 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.

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.
Architecture diagram
sequenceDiagram
    participant Client as Caller Component
    participant CB as Circular Buffer
    participant Alloc as Memory Allocator

    Client->>CB: cbuffer_add_unsafe(buf, d, d_len)

    CB->>CB: Compute used size len
    alt impossible: len ≥ max_size or d_len > max_size - len
        CB-->>Client: return 1 (error, no change)
    else possible
        loop while space insufficient (len ≥ size or d_len ≥ size - len)
            CB->>CB: cbuffer_realloc_unsafe(buf)
            alt growth succeeds
                CB->>Alloc: realloc (new_size = max_size or capped doubling, seed 1 if size 0)
                Alloc-->>CB: new buffer
                CB->>CB: move data, update size
            else growth fails
                CB-->>Client: return 1
            end
        end
        alt d_len < size - write
            CB->>CB: memcpy contiguous write
            CB->>CB: write += d_len
        else
            CB->>CB: memcpy wrap‑aware (two parts)
            CB->>CB: write updated with wrap
        end
        CB-->>Client: return 0
    end
    Note over Client,CB: Similar guards apply to cbuffer_reserve_unsafe and cbuffer_commit_reserved_unsafe (validation, safe wrap arithmetic)
Loading

Re-trigger cubic

@stelfrag
stelfrag marked this pull request as ready for review July 5, 2026 20:16
Copilot AI review requested due to automatic review settings July 5, 2026 20:16

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.

Pull request overview

Hardens libnetdata’s adaptive circular buffer to avoid infinite growth loops, integer overflow during resizing, and out-of-range write/commit arithmetic, while preserving behavior for valid callers.

Changes:

  • Make buffer growth overflow-safe and able to recover from a zero-sized buffer by seeding growth at 1 byte and capping before doubling.
  • Replace addition-based capacity checks (len + size) with subtraction-based bounds checks to avoid wraparound and impossible requests.
  • Make reserve/add/commit pointer arithmetic wrap-safe and add defensive validation in cbuffer_commit_reserved_unsafe().

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

@thiagoftsm 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.

No issues found during runtime. LGTM!

@stelfrag
stelfrag merged commit 908678e into netdata:master Jul 7, 2026
154 checks passed
@stelfrag
stelfrag deleted the netdata-fixes-33 branch July 7, 2026 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants