Skip to content

Commit ce7f21f

Browse files
ktsaouktsaou
committed
Refactor apps lookup semantic validation
1 parent a263a9b commit ce7f21f

4 files changed

Lines changed: 180 additions & 40 deletions

File tree

‎.agents/sow/current/SOW-0014-20260603-maintainability-hotspots.md‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,13 @@ Refined recommendation:
366366
- Continued the apps lookup builder complexity target:
367367
- reused the extracted lookup label writer from both cgroups and apps lookup builders in Go, C, and Rust.
368368
- kept item offsets, label-table layout, string storage order, directory entries, response finishing, and packed-data compaction unchanged.
369+
- User decision on 2026-06-04: proceed with option B, one more low-risk cleanup by splitting apps lookup semantic validation, then stop and re-check Codacy metrics.
370+
- Implemented option B by splitting apps lookup semantic validation into local domain, unknown-pid, and known-pid helpers in:
371+
- `src/go/pkg/netipc/protocol/lookup.go`
372+
- `src/libnetdata/netipc/src/protocol/netipc_protocol.c`
373+
- `src/crates/netipc/src/protocol/lookup.rs`
374+
- The apps lookup semantic validation split preserves the existing order: status domain, cgroup-status domain, comm length, unknown-pid zero-field checks, then known-pid cgroup-state checks.
375+
- The apps lookup semantic validation split intentionally did not change decode offsets, builder layout, label table storage, directory entries, response finishing, serialization, or public APIs.
369376

370377
## Validation
371378

@@ -429,6 +436,38 @@ Implementation validation:
429436
- ShellCheck: 0 issues.
430437
- Spectral: 0 issues.
431438
- total: 0 issues, 0 errors.
439+
- Option B apps lookup semantic validation split:
440+
- `gofmt -w src/go/pkg/netipc/protocol/lookup.go` passed.
441+
- `cargo fmt --manifest-path src/crates/netipc/Cargo.toml` passed.
442+
- Lizard focused lookup-code comparison:
443+
- Go `validateAppsLookupSemantics`: CCN 35 before, below the CCN 20 warning threshold after.
444+
- C `apps_lookup_validate_semantics`: CCN 35 before, below the CCN 20 warning threshold after.
445+
- Rust `validate_apps_lookup_semantics`: CCN 32 before, below the CCN 20 warning threshold after.
446+
- Local reproduction of the Go static-analysis lane passed in `src/go`:
447+
- `go test ./...` passed.
448+
- `go vet ./...` passed.
449+
- `staticcheck ./...` passed.
450+
- `govulncheck ./...` passed with no vulnerabilities.
451+
- `gosec -quiet -fmt sarif -out /tmp/plugin-ipc-gosec-src-go-b.sarif -exclude=G404 ./...` passed with status `0`.
452+
- `cargo test --manifest-path src/crates/netipc/Cargo.toml` passed: 329 Rust unit tests passed, plus bin/doc test targets.
453+
- `cmake --build build` passed.
454+
- `/usr/bin/ctest --test-dir build --output-on-failure -R protocol` passed: 4/4 protocol tests.
455+
- `bash tests/interop_codec.sh` passed after CTest, serially:
456+
- Rust decoded C output: 89 passed, 0 failed.
457+
- Go decoded C output: 90 passed, 0 failed.
458+
- C decoded Rust output: 101 passed, 0 failed.
459+
- Go decoded Rust output: 90 passed, 0 failed.
460+
- C decoded Go output: 101 passed, 0 failed.
461+
- Rust decoded Go output: 89 passed, 0 failed.
462+
- C, Rust, and Go generated byte-identical protocol fixture files, including all apps lookup response variants.
463+
- `codacy-analysis analyze --output-format json` passed:
464+
- Checkov: 0 issues.
465+
- Opengrep/Semgrep: 0 issues.
466+
- Trivy: 0 issues.
467+
- cppcheck: 0 issues.
468+
- ShellCheck: 0 issues.
469+
- Spectral: 0 issues.
470+
- total: 0 issues, 0 errors.
432471
- `bash .agents/sow/audit.sh` passed and reported SOW initialization complete and clean.
433472
- `git diff --check` passed.
434473
- Lookup codec complexity validation:
@@ -547,7 +586,7 @@ Sensitive data gate:
547586

548587
## Outcome
549588

550-
Raw cache, Go typed-facade, apps lookup, cgroups lookup, and the five Go code-scanning findings are locally remediated; the overall maintainability SOW remains in progress pending CI confirmation and the next useful target or closure decision.
589+
Raw cache, Go typed-facade, apps lookup builder, cgroups lookup builder, apps lookup semantic validation, and the five Go code-scanning findings are locally remediated; the overall maintainability SOW remains in progress pending CI confirmation and Codacy metric re-check.
551590

552591
## Lessons Extracted
553592

‎src/crates/netipc/src/protocol/lookup.rs‎

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,35 @@ fn validate_apps_lookup_semantics(
135135
path_len: u64,
136136
name_len: u64,
137137
label_count: u64,
138+
) -> Result<(), NipcError> {
139+
validate_apps_lookup_domains(status, cgroup_status, comm_len)?;
140+
if status == PID_LOOKUP_UNKNOWN {
141+
return validate_apps_lookup_unknown(
142+
orchestrator,
143+
cgroup_status,
144+
ppid,
145+
uid,
146+
starttime,
147+
comm_len,
148+
path_len,
149+
name_len,
150+
label_count,
151+
);
152+
}
153+
validate_apps_lookup_known(
154+
cgroup_status,
155+
orchestrator,
156+
comm_len,
157+
path_len,
158+
name_len,
159+
label_count,
160+
)
161+
}
162+
163+
fn validate_apps_lookup_domains(
164+
status: u16,
165+
cgroup_status: u16,
166+
comm_len: u64,
138167
) -> Result<(), NipcError> {
139168
if status != PID_LOOKUP_KNOWN && status != PID_LOOKUP_UNKNOWN {
140169
return Err(NipcError::BadLayout);
@@ -149,21 +178,43 @@ fn validate_apps_lookup_semantics(
149178
if comm_len > 15 {
150179
return Err(NipcError::BadLayout);
151180
}
152-
if status == PID_LOOKUP_UNKNOWN {
153-
if orchestrator != 0
154-
|| cgroup_status != 0
155-
|| ppid != 0
156-
|| uid != NIPC_UID_UNSET
157-
|| starttime != 0
158-
|| comm_len != 0
159-
|| path_len != 0
160-
|| name_len != 0
161-
|| label_count != 0
162-
{
163-
return Err(NipcError::BadLayout);
164-
}
165-
return Ok(());
181+
Ok(())
182+
}
183+
184+
fn validate_apps_lookup_unknown(
185+
orchestrator: u16,
186+
cgroup_status: u16,
187+
ppid: u32,
188+
uid: u32,
189+
starttime: u64,
190+
comm_len: u64,
191+
path_len: u64,
192+
name_len: u64,
193+
label_count: u64,
194+
) -> Result<(), NipcError> {
195+
if orchestrator != 0
196+
|| cgroup_status != 0
197+
|| ppid != 0
198+
|| uid != NIPC_UID_UNSET
199+
|| starttime != 0
200+
|| comm_len != 0
201+
|| path_len != 0
202+
|| name_len != 0
203+
|| label_count != 0
204+
{
205+
return Err(NipcError::BadLayout);
166206
}
207+
Ok(())
208+
}
209+
210+
fn validate_apps_lookup_known(
211+
cgroup_status: u16,
212+
orchestrator: u16,
213+
comm_len: u64,
214+
path_len: u64,
215+
name_len: u64,
216+
label_count: u64,
217+
) -> Result<(), NipcError> {
167218
if comm_len == 0 {
168219
return Err(NipcError::BadLayout);
169220
}

‎src/go/pkg/netipc/protocol/lookup.go‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,16 @@ func invalidSourceString(data []byte, requireNonEmpty bool) bool {
6464
}
6565

6666
func validateAppsLookupSemantics(status, cgroupStatus, orchestrator uint16, ppid, uid uint32, starttime uint64, commLen, pathLen, nameLen, labelCount int) error {
67+
if err := validateAppsLookupDomains(status, cgroupStatus, commLen); err != nil {
68+
return err
69+
}
70+
if status == PidLookupUnknown {
71+
return validateAppsLookupUnknown(orchestrator, cgroupStatus, ppid, uid, starttime, commLen, pathLen, nameLen, labelCount)
72+
}
73+
return validateAppsLookupKnown(cgroupStatus, orchestrator, commLen, pathLen, nameLen, labelCount)
74+
}
75+
76+
func validateAppsLookupDomains(status, cgroupStatus uint16, commLen int) error {
6777
if status != PidLookupKnown && status != PidLookupUnknown {
6878
return ErrBadLayout
6979
}
@@ -74,13 +84,18 @@ func validateAppsLookupSemantics(status, cgroupStatus, orchestrator uint16, ppid
7484
if commLen > 15 {
7585
return ErrBadLayout
7686
}
77-
if status == PidLookupUnknown {
78-
if orchestrator != 0 || cgroupStatus != 0 || ppid != 0 || uid != NipcUIDUnset ||
79-
starttime != 0 || commLen != 0 || pathLen != 0 || nameLen != 0 || labelCount != 0 {
80-
return ErrBadLayout
81-
}
82-
return nil
87+
return nil
88+
}
89+
90+
func validateAppsLookupUnknown(orchestrator, cgroupStatus uint16, ppid, uid uint32, starttime uint64, commLen, pathLen, nameLen, labelCount int) error {
91+
if orchestrator != 0 || cgroupStatus != 0 || ppid != 0 || uid != NipcUIDUnset ||
92+
starttime != 0 || commLen != 0 || pathLen != 0 || nameLen != 0 || labelCount != 0 {
93+
return ErrBadLayout
8394
}
95+
return nil
96+
}
97+
98+
func validateAppsLookupKnown(cgroupStatus, orchestrator uint16, commLen, pathLen, nameLen, labelCount int) error {
8499
if commLen == 0 {
85100
return ErrBadLayout
86101
}

‎src/libnetdata/netipc/src/protocol/netipc_protocol.c‎

Lines changed: 54 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -889,16 +889,9 @@ static bool source_string_invalid(const char *ptr, uint32_t len, bool require_no
889889
return ptr && bytes_have_nul(ptr, len);
890890
}
891891

892-
static nipc_error_t apps_lookup_validate_semantics(uint16_t status,
893-
uint16_t cgroup_status,
894-
uint16_t orchestrator,
895-
uint32_t ppid,
896-
uint32_t uid,
897-
uint64_t starttime,
898-
uint64_t comm_len,
899-
uint64_t cgroup_path_len,
900-
uint64_t cgroup_name_len,
901-
uint64_t label_count)
892+
static nipc_error_t apps_lookup_validate_domains(uint16_t status,
893+
uint16_t cgroup_status,
894+
uint64_t comm_len)
902895
{
903896
if (status != NIPC_PID_LOOKUP_KNOWN &&
904897
status != NIPC_PID_LOOKUP_UNKNOWN)
@@ -910,17 +903,35 @@ static nipc_error_t apps_lookup_validate_semantics(uint16_t status,
910903
return NIPC_ERR_BAD_LAYOUT;
911904
if (comm_len > 15)
912905
return NIPC_ERR_BAD_LAYOUT;
906+
return NIPC_OK;
907+
}
913908

914-
if (status == NIPC_PID_LOOKUP_UNKNOWN) {
915-
if (orchestrator != 0 || cgroup_status != 0 ||
916-
ppid != 0 || uid != NIPC_UID_UNSET ||
917-
starttime != 0 || comm_len != 0 ||
918-
cgroup_path_len != 0 || cgroup_name_len != 0 ||
919-
label_count != 0)
920-
return NIPC_ERR_BAD_LAYOUT;
921-
return NIPC_OK;
922-
}
909+
static nipc_error_t apps_lookup_validate_unknown(uint16_t orchestrator,
910+
uint16_t cgroup_status,
911+
uint32_t ppid,
912+
uint32_t uid,
913+
uint64_t starttime,
914+
uint64_t comm_len,
915+
uint64_t cgroup_path_len,
916+
uint64_t cgroup_name_len,
917+
uint64_t label_count)
918+
{
919+
if (orchestrator != 0 || cgroup_status != 0 ||
920+
ppid != 0 || uid != NIPC_UID_UNSET ||
921+
starttime != 0 || comm_len != 0 ||
922+
cgroup_path_len != 0 || cgroup_name_len != 0 ||
923+
label_count != 0)
924+
return NIPC_ERR_BAD_LAYOUT;
925+
return NIPC_OK;
926+
}
923927

928+
static nipc_error_t apps_lookup_validate_known(uint16_t cgroup_status,
929+
uint16_t orchestrator,
930+
uint64_t comm_len,
931+
uint64_t cgroup_path_len,
932+
uint64_t cgroup_name_len,
933+
uint64_t label_count)
934+
{
924935
if (comm_len == 0)
925936
return NIPC_ERR_BAD_LAYOUT;
926937

@@ -950,6 +961,30 @@ static nipc_error_t apps_lookup_validate_semantics(uint16_t status,
950961
return NIPC_OK;
951962
}
952963

964+
static nipc_error_t apps_lookup_validate_semantics(uint16_t status,
965+
uint16_t cgroup_status,
966+
uint16_t orchestrator,
967+
uint32_t ppid,
968+
uint32_t uid,
969+
uint64_t starttime,
970+
uint64_t comm_len,
971+
uint64_t cgroup_path_len,
972+
uint64_t cgroup_name_len,
973+
uint64_t label_count)
974+
{
975+
nipc_error_t err = apps_lookup_validate_domains(status, cgroup_status, comm_len);
976+
if (err != NIPC_OK)
977+
return err;
978+
if (status == NIPC_PID_LOOKUP_UNKNOWN)
979+
return apps_lookup_validate_unknown(orchestrator, cgroup_status,
980+
ppid, uid, starttime, comm_len,
981+
cgroup_path_len, cgroup_name_len,
982+
label_count);
983+
return apps_lookup_validate_known(cgroup_status, orchestrator,
984+
comm_len, cgroup_path_len,
985+
cgroup_name_len, label_count);
986+
}
987+
953988
static nipc_error_t cgroups_lookup_validate_semantics(uint16_t status,
954989
uint16_t orchestrator,
955990
uint64_t path_len,

0 commit comments

Comments
 (0)