Netdata fixes part 19 - #22921
Merged
Merged
Conversation
from_hex() used a plain char for two operations in URL percent decoding: it passed the byte directly to ctype helpers, and url_percent_escape_decode() left-shifted the returned char before forcing unsigned-byte arithmetic. On signed-char targets, malformed percent escapes with high-bit bytes can become negative values. Passing those values to isdigit()/tolower() violates the ctype contract, and left-shifting a negative promoted char is undefined behavior. This is a potential bug: normal URL decoding uses ASCII hex digits today, but malformed network-controlled input can still reach these helpers before the existing validation paths reject the decoded byte. Cast the input byte to uint8_t before the ctype calls, and cast the from_hex() results to uint8_t before the shift and OR. This keeps the public helper signatures, caller control flow, and validation behavior unchanged while making the malformed-byte arithmetic defined. Functional impact: none intended. Valid percent escapes decode identically, existing invalid ASCII escapes keep their prior behavior, and malformed high-bit escapes preserve the same resulting byte before the existing url_decode_r() validation paths decide whether to reject them. Performance impact: local casts only; no new branches, allocations, locks, loops, or function calls. Security impact: removes undefined behavior and possible ctype table out-of-bounds access on malformed high-bit input. Validation: git diff --check; searched URL and hex-combine call sites in the touched area; built src/libnetdata/url/url.c.o and libnetdata; ran a UBSan micro-check for the percent-escape combine expression. Required independent source reviews approved the surgical change and an adjacent UTF-8 signed-shift issue was recorded separately. (cherry picked from commit f6f42ee)
os_read_cpuset_cpus() cached a process-global scratch buffer and size while reading and parsing cpuset.cpus. The code defect is current shared mutable state without synchronization; runtime corruption is potential and requires concurrent callers such as daemon CPU config and cgroups CPU-limit refresh. Make the cache thread-local so each caller thread owns its grow-only read buffer. The parsing logic, file paths, return values, APIs, metrics, and configuration surface are unchanged. Functional impact: none intended. CPU count parsing remains identical for each thread, and callers only observe the returned size_t. Performance impact: no per-call allocation is added. The grow-only cache remains reused per thread and avoids a global lock around file I/O; memory changes from one process-wide scratch buffer to one small scratch buffer per calling thread. Security impact: removes a possible cross-thread read/write/use-after-free race on the scratch buffer and adds no new input surface. Validation: git diff --check; built CMakeFiles/libnetdata.dir/src/libnetdata/os/get_system_cpus.c.o; built libnetdata. Required independent source reviews approved the patch and found no blocker. (cherry picked from commit 5430313)
Issue: url_utf8_get_byte_length() counted UTF-8 leading one bits by left-shifting its char parameter. Percent-decoded UTF-8 start bytes such as 0xC3 are negative on signed-char platforms, so c <<= 1 is undefined behavior. Current/Potential impact: The undefined behavior is currently reachable through url_decode_r() -> url_decode_multibyte_utf8() for percent-encoded UTF-8 in web and ACLK URL decoding. Observed functional misbehavior is potential because common compilers emit the intended byte count today. Solution: Copy the input byte to uint8_t and shift that unsigned byte while preserving the existing API and return contract. Functional impact: No intended URL decode behavior changes. Byte-length results for ASCII, continuation bytes, invalid starts, and 2/3/4-byte UTF-8 starts are unchanged. Performance impact: Negligible. The change adds one local byte copy in the URL decode helper. Security impact: Removes external-input-reachable undefined behavior without adding parsing surface or privileges. Validation: - git diff --check - focused compile of src/libnetdata/url/url.c with min-build flags redirected to the worker source - UBSan micro-check for the 0xC3 shift path Source review: Required independent source reviews approved the diff as production grade and necessary. A non-blocking adjacent url_encode() signed right-shift note was recorded as a discovered finding. (cherry picked from commit 5aca1e7)
Issue: the query tail-fill loop advanced rrdr_line manually before writing r->o, r->v, and r->ar, unlike the normal row path that advances through the RRDR line guard. Classification: potential memory-safety robustness issue. Current valid-query invariants keep points_wanted aligned with r->n, but an invariant regression would leave this tail-fill writer without the same local bounds check as normal rows. Functional impact: no valid query output changes; tail-fill still writes the same empty flags, zero value, and zero anomaly rate to the same row order. Performance impact: negligible; the new helper is always-inline and adds the existing RRDR row bounds check to tail-filled rows. Solution: extract the guarded rrdr_line increment into rrdr_line_next(), keep rrdr_line_init() semantics unchanged, and use the helper before tail-fill RRDR array writes. (cherry picked from commit 02c1b6c)
RRD context flags are updated with seq-cst atomic helpers, but several live reads still used plain loads. The CAS helpers seeded expected from *flags before an atomic compare-exchange, and the post-processing/API snapshot paths read RRDCONTEXT, RRDINSTANCE, or RRDMETRIC flags directly while those objects can be updated concurrently. This is a current bug in the codebase: the affected objects are live and the flags are mutated through atomic RMW/CAS operations today, so mixing plain reads with those writes is C memory-model undefined behavior. On common hardware this is usually observed as the same value, but it leaves room for torn/stale snapshots or compiler assumptions around a raced object. Use __atomic_load_n(..., __ATOMIC_SEQ_CST) for the CAS seed reads and the existing rrd_flags_get()/rrd_flag_is_collected() helpers for live trigger, queue, and API v2 snapshots. Constructor-only, new-object, local aggregate, commented, and disabled-debug flag accesses are left unchanged because they are not live shared rrdcontext flag reads. Functional impact: no intended behavior change. The same flags are tested and copied, queued_flags keeps the same assignment/OR semantics, and the CAS helpers still return the pre-update flag value used by collected counter accounting. Performance impact: negligible. The CAS path already performs seq-cst compare-exchange, x86 seq-cst loads compile to ordinary loads, and the additional atomic snapshots are in trigger, queue, and API paths rather than per-sample collection loops. (cherry picked from commit 34ea74a)
|
Contributor
There was a problem hiding this comment.
No issues found across 9 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant API as API v2
participant RC as RRDCONTEXT
participant RI as RRDINSTANCE
participant RM as RRDMETRIC
participant PPQueue as PP Queue
participant Query as Query Engine
participant URL as URL Decoder
participant CPU as CPUset Parser
Note over API,CPU: Key runtime components for flag safety, URL decoding, query bounds, and thread safety
par API v2 Context Flow
API->>RC: rrdcontext_to_json_v2_add_context()
RC->>RC: rrd_flags_get(rc) - atomic snapshot
RC-->>API: context flags
API->>RI: iterate rrdinstances
RI->>RI: rrd_flag_is_collected(ri) - atomic read
alt window enabled
RI->>RI: query_matches_retention() with atomic flag
end
RI->>RM: iterate rrdmetrics
RM->>RM: rrd_flag_is_collected(rm) - atomic read
alt window enabled
RM->>RM: query_matches_retention() with atomic flag
end
end
par Trigger Updates Flow
Note over RC,Query: Update triggers queue context for post-processing
RC->>RC: rrdcontext_trigger_updates()
RC->>RC: rrd_flags_get(rc) - atomic snapshot
RC->>PPQueue: rrdcontext_queue_for_post_processing(flags snapshot)
Note over RI: Instance triggers
RI->>RI: rrdinstance_trigger_updates()
RI->>RI: rrd_flags_get(ri) - atomic snapshot
RI->>PPQueue: rrdcontext_queue_for_post_processing(flags snapshot)
Note over RM: Metric triggers
RM->>RM: rrdmetric_trigger_updates()
RM->>RM: rrd_flags_get(rm) - atomic snapshot
RM->>PPQueue: rrdcontext_queue_for_post_processing(flags snapshot)
Note over PPQueue: Queue stores queued_flags from atomic snapshot
PPQueue->>PPQueue: rrdcontext_add_to_pp_queue()
PPQueue->>PPQueue: rc->pp.queued_flags = rrd_flags_get(rc)
alt already queued
PPQueue->>PPQueue: queued_flags |= rrd_flags_get(rc)
end
end
par Query Execution Flow
Query->>Query: rrd2rrdr_query_execute()
Query->>Query: points_added loop
loop fill tail with empty values
Query->>Query: rrdr_line_next(r, rrdr_line)
Note over Query: Guarded increment with bounds check
Query->>Query: set RRDR_VALUE_EMPTY
end
alt final fill
Query->>Query: rrdr_line = rrdr_line_next(r, rrdr_line)
Query->>Query: same safe increment
end
end
par URL Decoding Flow
URL->>URL: url_percent_escape_decode()
URL->>URL: from_hex() with uint8_t cast
Note over URL: Unsigned byte operations for hex conversion
URL->>URL: url_utf8_get_byte_length()
URL->>URL: uint8_t byte = (uint8_t)c
Note over URL: Right-shift on unsigned copy for UTF-8 length
end
par CPUset Parsing Flow
Note over CPU: Thread-local buffers prevent cross-thread races
CPU->>CPU: os_read_cpuset_cpus()
CPU->>CPU: static __thread char *buf
CPU->>CPU: static __thread size_t buf_size
end
Note over API,CPU: Atomic operations use __atomic_load_n for compare-exchange helpers
Note over RC,PPQueue: rrd_flag_add_remove_atomic/rrd_flags_replace_atomic seed with atomic load
thiagoftsm
approved these changes
Jul 1, 2026
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
PR is working as expected, 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
Hardened flag handling, URL decoding, and query tail-fill to remove undefined behavior and data races without changing behavior. Improves safety in multi-threaded paths and malformed input handling.
Written for commit cc0b3e1. Summary will update on new commits.