Netdata fixes part 42 - #23009
Merged
Merged
Conversation
single_exponential_smoothing_reverse() initialized its cursor at the final element, consumed that element for the initial level, and then entered the loop with value++. The first loop iteration dereferenced series[entries], one element past the caller-provided array, and the loop could also form a pointer before the start of the array while terminating. Use a guarded reverse loop that decrements only while the cursor is above the first element. The helper now processes only valid preceding elements, preserves the existing entries==0 and alpha default behavior, and does not change any currently exercised Netdata path because the helper has no in-tree callers. (cherry picked from commit 2fa3573)
yaml_parse_config() built LOG2JOURNAL_CONFIG_PATH/<name>.yaml into a fixed FILENAME_MAX+1 buffer and ignored snprintf()'s return value. An overlong --config name could therefore pass a truncated filename to yaml_parse_file(), potentially opening a different file if that truncated path existed; this is a potential robustness issue for abnormal config names, not normal valid configs. Reject null and empty config names, and fail before opening when snprintf() errors or would truncate the generated path. Valid config names continue through the same yaml_parse_file() path. Functional impact: valid --config and --file behavior is unchanged; overlong or empty --config now fails with an explicit diagnostic instead of attempting a generated or truncated path. Performance impact: one snprintf return check and two cold-path branches during command-line config parsing only; no collection hot-path impact. Validation: built log2journal in a minimal local CMake build; git diff --cached --check passed; full src/collectors/log2journal/tests.sh passed with 82/82 tests. Reviewer verdicts: required reviewer process gate passed on attempt 1; semantic review approved with no blockers. Optional review also approved. A pre-existing --config path-separator behavior note was recorded as a separate decision-required side finding. (cherry picked from commit b502977)
c_rhash_get_uint8_by_str matched string keys without checking that the stored value_type was ITEMTYPE_UINT8. A same-key update through c_rhash_insert_str_ptr can replace the stored value with ITEMTYPE_OPAQUE_PTR, so the uint8 getter could return success with one byte of pointer storage. Classification: potential production impact, current helper type-confusion bug. Direct search found no production callers of the uint8 string getter, but the public c_rhash helper API can represent the mixed-type update. Solution: require ITEMTYPE_UINT8 before reading bin->value as uint8_t and add a focused mixed-type update regression test. Functional impact: valid uint8 string-key lookups are unchanged; mismatched value-type lookups now fail and clear the output instead of returning a misinterpreted byte. Performance impact: one extra value_type comparison while scanning this helper only. Security impact: reduces typed-value confusion in a low-level hash helper; no allocation, lifetime, or locking changes. Review: required reviewer gate approved; optional reviewer approved; no blocking source changes requested. Validation: git diff --check; c_rhash.c object compile against this worktree; c_rhash tests.c syntax compile. (cherry picked from commit c66b1b8)
Issue: GetDiskDriveInfo copied WMI BSTR disk properties into fixed char arrays through wcstombs. That conversion is locale-dependent and can fail or drop valid non-ASCII Windows strings when the process C locale cannot represent them; it also previously required every call site to handle truncation and termination correctly before later C-string consumers duplicate the fields. Current vs potential: ASCII WMI disk properties work today, but current Windows systems can expose non-ASCII DeviceID, Model, Caption, Name, Status, Manufacturer, InstallDate, or MediaType values. Those values currently depend on the process locale instead of the WMI UTF-16 source data, so valid metadata can be blanked or misconverted in real Windows environments. Solution: keep the file-local conversion helper, but make it call the existing utf16_to_utf8() helper instead of wcstombs. This uses the project Windows UTF-16 to UTF-8 conversion path, preserves bounded destination writes, clears failed conversions to an empty string, and keeps every destination NUL-terminated. Functional impact: ASCII and already representable values remain unchanged. Valid non-ASCII WMI strings are now emitted as UTF-8 instead of depending on the current C locale. Failed conversions still produce an empty field rather than partial or unterminated bytes. Performance impact: negligible. Disk metadata collection still performs one bounded conversion per WMI property, and this path is not a high-frequency metrics hot path. Security impact: positive. The change prevents C-string consumers from reading beyond fixed WMI metadata buffers and removes locale-dependent handling of UTF-16 WMI strings. Validation: git diff --check passed for src/libnetdata/os/windows-wmi/windows-wmi-GetDiskDriveInfo.c. A debugfs-enabled CMake configure passed after initializing submodules, cmake --build for debugfs.plugin passed, and the build compiled src/libnetdata/os/windows-wmi/windows-wmi-GetDiskDriveInfo.c plus src/libnetdata/string/utf8.c into libnetdata. Native Windows runtime validation was not run in this Linux slot. Reviewer gate: required local semantic review gate passed for the original finding, and the online reviewer follow-up was verified against the source before this fixup. (cherry picked from commit 9eb0241)
Issue: the json-c boolean helper set JSON_ENTRY.type and JSON_ENTRY.data.boolean but left JSON_ENTRY.name unchanged. The follow-up fix copied the current key like the adjacent string and integer helpers, but it used strlen() before applying the JSON_NAME_LEN cap, so it computed an unbounded source length even though only a bounded callback name is needed. Current vs potential: current json-c object iteration normally supplies a key for object members, and current in-tree boolean callback consumers do not branch on boolean names. The source-level bug is still real for generic json_parse callbacks because boolean entries could carry stale names, a NULL key must be handled, and a very long key should not require a full strlen() scan before the bounded copy. Solution: copy the current json-c object key into e->name for boolean callbacks, treat a NULL key as an empty name, and use strnlen(key, JSON_NAME_LEN) so the length calculation is bounded by the destination name capacity. The helper performs memcpy() only when there are bytes to copy. Functional impact: boolean callbacks now receive their actual key name when one is present. Parser traversal, callback ordering, ownership/lifetime, and NULL-key fallback semantics remain bounded and deterministic; a missing key becomes an empty callback name. Performance impact: valid boolean object members now pay a bounded strnlen, at most one bounded memcpy, and one terminator write, matching the adjacent scalar helper behavior while avoiding an unbounded scan. The extra NULL check is negligible. Security impact: removes stale callback metadata for boolean fields, avoids NULL-key dereference in the helper, and bounds the key length scan by JSON_NAME_LEN. The copy remains bounded and introduces no allocation, input acceptance, or buffer growth change. Validation: git diff --check passed for src/libnetdata/json/json.c and src/libnetdata/json/json-c-parser-unittest.c. A debugfs-enabled CMake configure passed after initializing submodules. cmake --build for debugfs.plugin passed and focused builds recompiled src/libnetdata/json/json.c into libnetdata. Reviewer gate: required three-reviewer process gate passed for the original worker finding, and follow-up static-analysis feedback was verified against the source before this fixup. (cherry picked from commit ff0772b)
The /api/v3/settings contract documents a 20 MiB JSON payload limit, and the GET path already refuses to read stored settings larger than that limit. The PUT path accepted any non-empty payload that reached the endpoint and passed it to JSON-C before an endpoint size check, so a complete oversized PUT could consume memory and CPU and then write a file beyond the documented contract. This is a current source bug for complete oversized PUT requests that bypass the incomplete-request transport guard, including keep-alive flows where the reusable response buffer has already grown. Add the same payload-size boundary to the PUT dispatcher before settings_put(), JSON parsing, file I/O, or the settings write lock. Functional impact: valid payloads up to and including MAX_SETTINGS_SIZE_BYTES keep the existing path and errors. Only payloads above the documented API limit now return HTTP 413. Performance impact: valid PUT requests add one O(1) buffer length read. Oversized requests avoid JSON parsing, serialization, file I/O, and lock acquisition. Security impact: reduces oversized-payload memory and CPU pressure at the settings endpoint. Validation: git diff --check -- src/web/api/v3/api_v3_settings.c; cmake --build /tmp/netdata-mem-corruptions-pr-comments-debugfs-build --target src/web/api/v3/api_v3_settings.c.o -j 4. Reviewer gate: required reviewers approved the final one-file diff. (cherry picked from commit c4c9a5f)
Issue: stream_circular_buffer_add_unsafe() forced autoscale used max_size * 2 without checking size_t overflow. If max_size exceeded SIZE_MAX / 2, the product could wrap smaller and force=true would accept a shrink while trying to grow. Current vs potential: normal deployments start from MB-scale limits and would exhaust memory long before the boundary, so this is a potential source-level bug rather than an observed live failure. The arithmetic path is still real and violates the autoscale invariant. Solution: saturate forced autoscale growth to SIZE_MAX and call the setter only when the computed value is larger than the current max_size. Functional impact: normal growth remains unchanged for max_size <= SIZE_MAX / 2. Sender paths with autoscale disabled are unchanged. No public command, configuration, protocol, or operator surface changes. Performance impact: adds one comparison and one branch on the existing unlikely autoscale path only; no new allocation, copying, locking, or atomic operation. Security impact: prevents a theoretical denial-of-service/disconnect path from wrapped forced growth shrinking the logical buffer maximum. Validation: git diff --check passed; CMake configure passed with optional xenstat and debugfs plugins disabled; src/streaming/stream-circular-buffer.c compiled successfully using the generated compile command. Direct Ninja object build was blocked by an unrelated missing generated ACLK protobuf input during global re-check. Independent source-review approval: required adversarial reviewers semantically approved the source diff as production grade and absolutely necessary with no blockers. (cherry picked from commit 52ff02a)
MEM-locks-004 is a potential data-race finding: completion_reset() cleared completed and completed_jobs without the mutex used by every other completion accessor. Current vs potential: completion_reset() is exercised today by ACLK and metadata startup code, but source review found current callers reset after completion_wait_for() returns and before shutdown reuse. No deterministic current concurrent reset/mark/wait interleaving was found, so the defect is classified as potential in the current codebase. Functional impact: no intended user-visible behavior change. The same fields are reset to the same values; reset now follows the completion primitive's existing synchronization contract. Performance impact: two uncontended mutex operations on a cold reset path with two current startup-time callers. No benchmark is required for this path. Solution: take p->mutex while clearing the shared completion state so reset uses the same lock as completion_wait_for(), completion_mark_complete(), completion_wait_for_a_job(), completion_mark_complete_a_job(), and completion_is_done(). (cherry picked from commit 7f49791)
Issue: MCP websocket close and disconnect callbacks both owned the same MCP_CLIENT pointer through websocket user_data and duplicated the cleanup sequence. The old sequence freed the MCP client before clearing user_data, so repeated or reordered lifecycle callbacks depended on the later NULL write and left a stale pointer visible during mcp_free_client(). Current/potential classification: potential. Current websocket teardown is serialized on the websocket thread and mcp_free_client() does not reenter websocket callbacks, so no current crash path was proven. The lifecycle surface is still real because both callbacks can run for the same websocket client and the cleanup pattern was duplicated. Functional impact: preserves teardown behavior. The same MCP context is freed exactly once from the same close/disconnect callbacks; user_data is only detached before the free instead of after it. Performance impact: none in steady state. The only added cost is a static helper call during websocket teardown. Solution: move MCP websocket context release into one static helper that returns on NULL, clears user_data first, and then frees the MCP client. (cherry picked from commit 9f765bf)
Root cause/current issue: the libunwind stacktrace backend reported async-signal-safety for dynamic builds even though Netdata does not install a signal-safe program-header iterator and libunwind's default path can use dl_iterate_phdr(), which is not async-signal-safe. Current vs potential status: valid potential bug. The validated pure-libunwind CMake configuration does not compile the current deadly-signal stacktrace caller, but the source contract was false and would allow an unsafe signal-handler call if that caller becomes active for libunwind. Solution: make the libunwind backend report false for signal-handler stacktrace capture and remove the adjacent stale comment claiming stacktrace_capture() was async-signal-safe. Functional impact: non-signal stacktrace capture is unchanged. Signal-handler code that consults this contract with an active libunwind backend will use the existing unsafe-backend fallback instead of attempting libunwind capture. Performance impact: no steady-state impact. When the contract is consulted from a signal handler, the path avoids libunwind unwinding and symbolization work. Bug status: fixed for the assigned libunwind signal-safety contract. (cherry picked from commit 69fff6a)
duration_parse_seconds() parses through the int64_t duration parser, but then stored the result into an int without checking that the parsed value fits. Inputs larger than INT_MAX seconds or smaller than INT_MIN seconds were reported as successful while silently narrowing to an unrelated int value. Current vs potential bug: this is source-backed and currently reachable from health, logging, inicfg, and duration reformatting paths that call duration_parse_seconds(). It requires an out-of-int-range duration value in configuration or input text, so normal in-range durations are unaffected. Solution: reject parsed values outside INT_MIN..INT_MAX before assigning to the int result. health_parse_repeat() now parses into a local int and assigns explicitly to the uint32_t destination, removing the previous casted write through an incompatible int pointer. Functional impact: valid in-range durations keep the same behavior. Invalid out-of-int-range durations now follow existing failure/default/logging paths instead of being accepted after truncation. The health repeat conversion preserves the existing destination type and assignment behavior for parsed in-range values. Performance impact: one constant-time bounds check is added to a low-frequency parsing helper. There is no new allocation, locking, I/O, or hot-path work. Validation: inspected the staged diff. Worker validation passed whitespace checks, focused duration parser tests, and a health_config object build; broader build attempts were blocked by unrelated optional dependency/generated-file issues. Required local reviewer quorum approved the source-level fix. (cherry picked from commit 5f7cef2)
stacktrace_to_buffer() lazily publishes st->text while holding stacktrace_lock, but the cache-hit path read the pointer without the same synchronization. Concurrent conversion of the same interned stacktrace could therefore read a pointer while another thread writes it. This is a potential runtime data race with deterministic source evidence: the current code has a write-under-lock/read-without-lock pattern on the cached text pointer. The cached string is published once and source review found no free or reassignment path after publication. Load st->text under stacktrace_lock, release the lock, and append the copied pointer afterward. The lock is intentionally not held across buffer_strcat(), backend symbol resolution, caller-buffer growth, or formatting. Functional impact: no intended output change. Cache hits still append the same cached text, and cache misses still use the existing backend conversion and lazy publish path. Performance impact: one short stacktrace_lock acquire/release per stacktrace_to_buffer() call. The lock hold is pointer-only and avoids allocation or buffer work under the spinlock. Validation: git diff --check passed; the worker configured and built libnetdata with low priority. Full netdata/unit-test execution was blocked by an unrelated missing ACLK proto source. (cherry picked from commit 598db34)
get_release_channel() lazily cached the release channel in a function-static int with plain reads and writes. Concurrent /api/v1/charts requests, analytics collection, or ACLK node-info generation could overlap on first use and race on that cache. This is a potential bug: source review proves concurrent web reachability through the static-threaded web server and /api/v1/charts, but no runtime crash or corruption reproducer was part of the finding evidence. Protect first initialization with a function-local spinlock and publish the cache with release/acquire atomics. The slow path still reads the same .environment file and falls back to the same NETDATA_VERSION heuristic; the fast path reads the initialized value without taking the lock. Functional impact: none intended. The function still returns the same stable/nightly string literals, the JSON release_channel field, analytics value, and ACLK node-info value keep the same semantics, and callers still copy or immediately serialize the returned string. Performance impact: first initialization serializes the existing one-time file parse. Steady state changes from a plain cached load to one atomic acquire load, with no lock, allocation, or file I/O after initialization. Validation: git diff --cached --check passed. Worker validation passed low-priority CMake configure and a targeted charts2json object compile. Local source-review gate verified all callers, callee lifetime/ownership, and the local spinlock/atomic pattern. (cherry picked from commit 707d05b)
github-actions
Bot
added
area/web
area/health
area/collectors
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 19 files
Confidence score: 2/5
- In
src/collectors/log2journal/log2journal-yaml.c, the--configvalue is interpolated into"%s/%s.yaml"without blocking..or path separators, so a crafted input can traverse directories and load unintended YAML files. That creates a concrete config-injection surface if merged as-is — validateconfig_nameagainst a strict allowlist (or canonicalize and enforce base-dir confinement) before merging. - In
src/libnetdata/c_rhash/tests.c, the new assertion expectsval == 0afterc_rhash_get_uint8_by_str()returns failure, butvalremains the pre-set123when the function does not write to*ret_val; this makes the test fail deterministically. Merging now is likely to break CI and obscure signal from real regressions — update the assertion (or function contract) to match actual failure-path behavior before merge. - In
src/web/api/formatters/charts2json.c, the addedSPINLOCKspans.environmentfile I/O (procfile_open()/procfile_readall()), so concurrent/api/v1/chartsrequests can busy-wait under load. The likely consequence is avoidable CPU burn and latency spikes during first-read contention — switch to a mutex/once-style initialization or move I/O outside the spin-locked region before merging.
Prompt for AI agents (unresolved issues)
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/web/api/formatters/charts2json.c">
<violation number="1" location="src/web/api/formatters/charts2json.c:14">
P2: Concurrent `/api/v1/charts` requests can spin while the first request performs `.environment` filesystem I/O. A sleep/spin `SPINLOCK` is now held across `procfile_open()`/`procfile_readall()`; a mutex/once primitive would avoid tying web worker CPU wait to blocking I/O.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Caller as Caller Code
participant JSON as JSON Parser
participant HashMap as c_rhash
participant Completion as Completion
participant StreamBuf as Stream Circular Buffer
participant ReleaseCh as Release Channel
Note over Caller,ReleaseCh: Bug fixes across subsystems
rect rgb(240, 248, 255)
Note over Caller,JSON: JSON boolean key propagation
Caller->>JSON: json_parse(payload, callback)
JSON->>JSON: For each boolean, NEW: set e->name from key
JSON-->>Caller: callback(entry) with name populated
Note over Caller: Previously e->name was empty for booleans
end
rect rgb(255, 240, 240)
Note over Caller,HashMap: Hash map value type enforcement
Caller->>HashMap: c_rhash_get_uint8_by_str(hash, key, &val)
HashMap->>HashMap: Iterate bins
alt bin->value_type != ITEMTYPE_UINT8
HashMap-->>Caller: return error (non-zero)
else Correct type
HashMap-->>Caller: return 0, val set
end
end
rect rgb(240, 255, 240)
Note over Caller,Completion: Completion reset with mutex
Caller->>Completion: completion_reset(&p)
Completion->>Completion: CHANGED: netdata_mutex_lock
Completion->>Completion: reset fields
Completion->>Completion: netdata_mutex_unlock
end
rect rgb(255, 255, 240)
Note over Caller,StreamBuf: Streaming buffer autoscale overflow guard
Caller->>StreamBuf: stream_circular_buffer_add_unsafe(..., autoscale=true)
StreamBuf->>StreamBuf: Check available size < needed
alt max_size <= SIZE_MAX/2
StreamBuf->>StreamBuf: NEW: max_size *= 2
else max_size > SIZE_MAX/2
StreamBuf->>StreamBuf: NEW: cap at SIZE_MAX, skip if no increase
end
StreamBuf-->>Caller: result
end
rect rgb(240, 240, 255)
Note over Caller,ReleaseCh: Release channel thread-safe caching
Caller->>ReleaseCh: get_release_channel()
ReleaseCh->>ReleaseCh: CHANGED: atomic load (acquire)
alt stable == -1
ReleaseCh->>ReleaseCh: spinlock_lock, recheck
ReleaseCh->>ReleaseCh: parse .environment (locked)
ReleaseCh->>ReleaseCh: atomic store (release), spinlock_unlock
end
ReleaseCh-->>Caller: "stable" or "nightly"
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a grab-bag of hardening fixes across the Agent C codebase, focusing on preventing edge-case misbehavior (bounds/overflow, concurrency, cleanup ordering) and adding regression tests for recently tightened parsing and JSON-walk behavior.
Changes:
- Tightens parsing and input validation (settings PUT payload cap, duration seconds int-range enforcement, log2journal
--configname validation). - Fixes correctness/safety issues in runtime components (WebSocket MCP context lifecycle cleanup, circular buffer autoscale overflow guard, reverse smoothing bounds, WMI UTF-16→UTF-8 conversion).
- Improves concurrency safety and adds/extends targeted unit tests (stacktrace cached text access, completion reset locking, json boolean key propagation, rhash value type enforcement).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/mcp/adapters/mcp-websocket.c | Centralizes and hardens WebSocket MCP context cleanup to avoid stale user_data. |
| src/web/api/v3/api_v3_settings.c | Rejects oversized settings PUT payloads (413) before parsing/applying. |
| src/web/api/formatters/charts2json.c | Makes get_release_channel() initialization thread-safe (spinlock + atomics). |
| src/streaming/stream-circular-buffer.c | Prevents size_t overflow when autoscaling the circular buffer max size. |
| src/libnetdata/statistical/statistical.c | Fixes reverse smoothing loop bounds to avoid out-of-range pointer behavior. |
| src/libnetdata/stacktrace/stacktrace-libunwind.c | Corrects async-signal-safety reporting for libunwind capture. |
| src/libnetdata/stacktrace/stacktrace-common.c | Synchronizes access to cached stacktrace text with stacktrace_lock. |
| src/libnetdata/parsers/duration.c | Rejects durations that overflow int when parsing into seconds. |
| src/libnetdata/parsers/duration-unittest.c | Adds regression tests for duration_parse_seconds() int-range behavior. |
| src/libnetdata/os/windows-wmi/windows-wmi-GetDiskDriveInfo.c | Uses bounded UTF-16→UTF-8 conversion to preserve non-ASCII disk metadata safely. |
| src/libnetdata/json/json.c | Ensures boolean JSON-walk callbacks include the correct key name. |
| src/libnetdata/json/json-c-parser-unittest.c | Adds a unit test to prevent regressions in boolean key propagation during JSON walk. |
| src/libnetdata/completion/completion.c | Serializes completion_reset() with the completion mutex. |
| src/libnetdata/c_rhash/tests.c | Adds regression coverage for uint8 lookups when keys exist with a different value type. |
| src/libnetdata/c_rhash/c_rhash.c | Enforces uint8 value type checks during uint8 lookups. |
| src/health/health_config.c | Validates health repeat intervals (rejects negative durations, avoids unsafe casts). |
| src/collectors/log2journal/tests.d/error-config-name-too-long.fail | Adds expected stderr substring for overlong --config name test. |
| src/collectors/log2journal/tests.d/error-config-name-too-long.cmd | Adds a failure test invoking log2journal with an extremely long -c value. |
| src/collectors/log2journal/log2journal-yaml.c | Rejects empty config names and detects/handles config filename truncation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thiagoftsm
approved these changes
Jul 7, 2026
thiagoftsm
left a comment
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
PR is running as expected on Windows. LGTM!
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by cubic
Strengthens safety and correctness across config parsing, JSON handling, Windows WMI, streaming buffers, and concurrency. Adds bounds checks, proper locking, and focused tests; enforces settings payload limits and fixes boolean key propagation.
--configin log2journal; reject out‑of‑int‑range durations and negative health repeats.completion_reset, synchronize stacktrace cached text reads, and make release channel cache thread‑safe.user_databefore freeing client and centralize cleanup.uint8value type in lookups; adds regression test.Written for commit db31341. Summary will update on new commits.