Skip to content

Commit 47f90ea

Browse files
committed
Refactor apps lookup semantic validation
1 parent ef9fcef commit 47f90ea

4 files changed

Lines changed: 265 additions & 295 deletions

File tree

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,15 @@ Refined recommendation:
340340
- `src/go/pkg/netipc/service/apps_lookup/client_common.go`
341341
- `src/go/pkg/netipc/service/cgroups_lookup/client_common.go`
342342
- Left build-tagged Go files responsible only for POSIX/Windows transport config conversion and platform-specific `NewCache()` construction where needed.
343+
- Committed and pushed the typed-facade cleanup as `ef9fcef` with message `Refactor Go service facade wrappers`.
344+
- GitHub reported the direct push bypassed the pull-request rule for `main`, as requested by the user.
345+
- Switched to the lookup codec complexity target.
346+
- Extracted apps lookup semantic validation into one local helper per language:
347+
- `src/go/pkg/netipc/protocol/lookup.go`
348+
- `src/libnetdata/netipc/src/protocol/netipc_protocol.c`
349+
- `src/crates/netipc/src/protocol/lookup.rs`
350+
- The codec change intentionally did not change layout offsets, byte-order writes, label table construction, directory entries, or packed-data compaction.
351+
- The extracted helpers preserve the existing validation order: status domain, cgroup-status domain, comm length, unknown-pid metadata rules, known-pid/cgroup-state rules, then source-string validation in builder paths.
343352

344353
## Validation
345354

@@ -405,6 +414,42 @@ Implementation validation:
405414
- total: 0 issues, 0 errors.
406415
- `bash .agents/sow/audit.sh` passed and reported SOW initialization complete and clean.
407416
- `git diff --check` passed.
417+
- Lookup codec complexity validation:
418+
- `gofmt -w src/go/pkg/netipc/protocol/lookup.go` passed.
419+
- `cargo fmt --manifest-path src/crates/netipc/Cargo.toml` passed.
420+
- `go test ./pkg/netipc/protocol` passed.
421+
- `go test ./...` passed in `src/go`.
422+
- `cargo test --manifest-path src/crates/netipc/Cargo.toml` passed: 329 Rust unit tests passed, plus bin/doc test targets.
423+
- `cmake --build build` passed.
424+
- `/usr/bin/ctest --test-dir build --output-on-failure -R protocol` passed: 4/4 protocol tests.
425+
- `bash tests/interop_codec.sh` passed after CTest, serially:
426+
- Rust decoded C output: 89 passed, 0 failed.
427+
- Go decoded C output: 90 passed, 0 failed.
428+
- C decoded Rust output: 101 passed, 0 failed.
429+
- Go decoded Rust output: 90 passed, 0 failed.
430+
- C decoded Go output: 101 passed, 0 failed.
431+
- Rust decoded Go output: 89 passed, 0 failed.
432+
- C, Rust, and Go generated byte-identical protocol fixture files.
433+
- Lizard focused lookup-code comparison:
434+
- Go `decodeAppsLookupItem`: CCN 52 before, 19 after.
435+
- Go `AppsLookupBuilder.Add`: CCN 71 before, 39 after.
436+
- C `apps_lookup_decode_item_bytes`: CCN 49 before, 16 after.
437+
- C `nipc_apps_lookup_builder_add`: CCN 63 before, 31 after.
438+
- Rust `decode_apps_item`: CCN 43 before, 13 after.
439+
- Rust `AppsLookupBuilder::add`: CCN 56 before, 27 after.
440+
- New semantic helpers remain intentionally high-complexity state machines: Go CCN 35, C CCN 35, Rust CCN 32.
441+
- Focused lookup-code average CCN moved from 7.4 to 6.8; NLOC warning ratio moved from 0.32 to 0.26.
442+
- `codacy-analysis analyze --output-format json` passed:
443+
- Checkov: 0 issues.
444+
- Opengrep/Semgrep: 0 issues.
445+
- Trivy: 0 issues.
446+
- cppcheck: 0 issues.
447+
- ShellCheck: 0 issues.
448+
- Spectral: 0 issues.
449+
- total: 0 issues, 0 errors.
450+
- `bash .agents/sow/audit.sh` passed and reported SOW initialization complete and clean.
451+
- `git diff --check` passed.
452+
- Sensitive-data scan across the touched SOW and codec files returned no matches.
408453

409454
Sensitive data gate:
410455

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

Lines changed: 96 additions & 117 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,75 @@ fn source_string_invalid(bytes: &[u8], require_non_empty: bool) -> bool {
124124
(require_non_empty && bytes.is_empty()) || bytes.contains(&0)
125125
}
126126

127+
fn validate_apps_lookup_semantics(
128+
status: u16,
129+
cgroup_status: u16,
130+
orchestrator: u16,
131+
ppid: u32,
132+
uid: u32,
133+
starttime: u64,
134+
comm_len: u64,
135+
path_len: u64,
136+
name_len: u64,
137+
label_count: u64,
138+
) -> Result<(), NipcError> {
139+
if status != PID_LOOKUP_KNOWN && status != PID_LOOKUP_UNKNOWN {
140+
return Err(NipcError::BadLayout);
141+
}
142+
if cgroup_status != APPS_CGROUP_KNOWN
143+
&& cgroup_status != APPS_CGROUP_UNKNOWN_RETRY_LATER
144+
&& cgroup_status != APPS_CGROUP_UNKNOWN_PERMANENT
145+
&& cgroup_status != APPS_CGROUP_HOST_ROOT
146+
{
147+
return Err(NipcError::BadLayout);
148+
}
149+
if comm_len > 15 {
150+
return Err(NipcError::BadLayout);
151+
}
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(());
166+
}
167+
if comm_len == 0 {
168+
return Err(NipcError::BadLayout);
169+
}
170+
match cgroup_status {
171+
APPS_CGROUP_KNOWN => {
172+
if path_len == 0 {
173+
return Err(NipcError::BadLayout);
174+
}
175+
}
176+
APPS_CGROUP_UNKNOWN_RETRY_LATER => {
177+
if orchestrator != 0 || name_len != 0 || label_count != 0 {
178+
return Err(NipcError::BadLayout);
179+
}
180+
}
181+
APPS_CGROUP_UNKNOWN_PERMANENT => {
182+
if path_len == 0 || orchestrator != 0 || name_len != 0 || label_count != 0 {
183+
return Err(NipcError::BadLayout);
184+
}
185+
}
186+
APPS_CGROUP_HOST_ROOT => {
187+
if orchestrator != 0 || path_len != 0 || name_len != 0 || label_count != 0 {
188+
return Err(NipcError::BadLayout);
189+
}
190+
}
191+
_ => return Err(NipcError::BadLayout),
192+
}
193+
Ok(())
194+
}
195+
127196
fn validate_lookup_dir(
128197
buf: &[u8],
129198
dir_start: usize,
@@ -1016,59 +1085,18 @@ fn decode_apps_item(item: &[u8]) -> Result<AppsLookupItemView<'_>, NipcError> {
10161085
if u16_at(item, 0) != 1 || u32_at(item, 20) != 0 || u16_at(item, 58) != 0 {
10171086
return Err(NipcError::BadLayout);
10181087
}
1019-
if status != PID_LOOKUP_KNOWN && status != PID_LOOKUP_UNKNOWN {
1020-
return Err(NipcError::BadLayout);
1021-
}
1022-
if cgroup_status != APPS_CGROUP_KNOWN
1023-
&& cgroup_status != APPS_CGROUP_UNKNOWN_RETRY_LATER
1024-
&& cgroup_status != APPS_CGROUP_UNKNOWN_PERMANENT
1025-
&& cgroup_status != APPS_CGROUP_HOST_ROOT
1026-
{
1027-
return Err(NipcError::BadLayout);
1028-
}
1029-
if comm_len > 15 {
1030-
return Err(NipcError::BadLayout);
1031-
}
1032-
if status == PID_LOOKUP_UNKNOWN {
1033-
if orchestrator != 0
1034-
|| cgroup_status != 0
1035-
|| ppid != 0
1036-
|| uid != NIPC_UID_UNSET
1037-
|| starttime != 0
1038-
|| comm_len != 0
1039-
|| path_len != 0
1040-
|| name_len != 0
1041-
|| label_count != 0
1042-
{
1043-
return Err(NipcError::BadLayout);
1044-
}
1045-
} else if comm_len == 0 {
1046-
return Err(NipcError::BadLayout);
1047-
} else {
1048-
match cgroup_status {
1049-
APPS_CGROUP_KNOWN => {
1050-
if path_len == 0 {
1051-
return Err(NipcError::BadLayout);
1052-
}
1053-
}
1054-
APPS_CGROUP_UNKNOWN_RETRY_LATER => {
1055-
if orchestrator != 0 || name_len != 0 || label_count != 0 {
1056-
return Err(NipcError::BadLayout);
1057-
}
1058-
}
1059-
APPS_CGROUP_UNKNOWN_PERMANENT => {
1060-
if path_len == 0 || orchestrator != 0 || name_len != 0 || label_count != 0 {
1061-
return Err(NipcError::BadLayout);
1062-
}
1063-
}
1064-
APPS_CGROUP_HOST_ROOT => {
1065-
if orchestrator != 0 || path_len != 0 || name_len != 0 || label_count != 0 {
1066-
return Err(NipcError::BadLayout);
1067-
}
1068-
}
1069-
_ => return Err(NipcError::BadLayout),
1070-
}
1071-
}
1088+
validate_apps_lookup_semantics(
1089+
status,
1090+
cgroup_status,
1091+
orchestrator,
1092+
ppid,
1093+
uid,
1094+
starttime,
1095+
comm_len as u64,
1096+
path_len as u64,
1097+
name_len as u64,
1098+
label_count as u64,
1099+
)?;
10721100

10731101
let (comm, comm_end) = lookup_string(item, APPS_LOOKUP_ITEM_HDR_SIZE, comm_off, comm_len)?;
10741102
let (cgroup_path, path_end) =
@@ -1168,77 +1196,28 @@ impl<'a> AppsLookupBuilder<'a> {
11681196
self.error = Some(NipcError::Overflow);
11691197
return Err(NipcError::Overflow);
11701198
}
1171-
if status != PID_LOOKUP_KNOWN && status != PID_LOOKUP_UNKNOWN {
1172-
self.error = Some(NipcError::BadLayout);
1173-
return Err(NipcError::BadLayout);
1174-
}
1175-
if cgroup_status != APPS_CGROUP_KNOWN
1176-
&& cgroup_status != APPS_CGROUP_UNKNOWN_RETRY_LATER
1177-
&& cgroup_status != APPS_CGROUP_UNKNOWN_PERMANENT
1178-
&& cgroup_status != APPS_CGROUP_HOST_ROOT
1179-
{
1180-
self.error = Some(NipcError::BadLayout);
1181-
return Err(NipcError::BadLayout);
1199+
if let Err(err) = validate_apps_lookup_semantics(
1200+
status,
1201+
cgroup_status,
1202+
orchestrator,
1203+
ppid,
1204+
uid,
1205+
starttime,
1206+
comm.len() as u64,
1207+
cgroup_path.len() as u64,
1208+
cgroup_name.len() as u64,
1209+
labels.len() as u64,
1210+
) {
1211+
self.error = Some(err);
1212+
return Err(err);
11821213
}
1183-
if comm.len() > 15
1184-
|| source_string_invalid(comm, status == PID_LOOKUP_KNOWN)
1214+
if source_string_invalid(comm, status == PID_LOOKUP_KNOWN)
11851215
|| source_string_invalid(cgroup_path, false)
11861216
|| source_string_invalid(cgroup_name, false)
11871217
{
11881218
self.error = Some(NipcError::BadLayout);
11891219
return Err(NipcError::BadLayout);
11901220
}
1191-
if status == PID_LOOKUP_UNKNOWN {
1192-
if orchestrator != 0
1193-
|| cgroup_status != 0
1194-
|| ppid != 0
1195-
|| uid != NIPC_UID_UNSET
1196-
|| starttime != 0
1197-
|| !comm.is_empty()
1198-
|| !cgroup_path.is_empty()
1199-
|| !cgroup_name.is_empty()
1200-
|| !labels.is_empty()
1201-
{
1202-
self.error = Some(NipcError::BadLayout);
1203-
return Err(NipcError::BadLayout);
1204-
}
1205-
} else {
1206-
match cgroup_status {
1207-
APPS_CGROUP_KNOWN => {
1208-
if cgroup_path.is_empty() {
1209-
self.error = Some(NipcError::BadLayout);
1210-
return Err(NipcError::BadLayout);
1211-
}
1212-
}
1213-
APPS_CGROUP_UNKNOWN_RETRY_LATER => {
1214-
if orchestrator != 0 || !cgroup_name.is_empty() || !labels.is_empty() {
1215-
self.error = Some(NipcError::BadLayout);
1216-
return Err(NipcError::BadLayout);
1217-
}
1218-
}
1219-
APPS_CGROUP_UNKNOWN_PERMANENT => {
1220-
if cgroup_path.is_empty()
1221-
|| orchestrator != 0
1222-
|| !cgroup_name.is_empty()
1223-
|| !labels.is_empty()
1224-
{
1225-
self.error = Some(NipcError::BadLayout);
1226-
return Err(NipcError::BadLayout);
1227-
}
1228-
}
1229-
APPS_CGROUP_HOST_ROOT => {
1230-
if orchestrator != 0
1231-
|| !cgroup_path.is_empty()
1232-
|| !cgroup_name.is_empty()
1233-
|| !labels.is_empty()
1234-
{
1235-
self.error = Some(NipcError::BadLayout);
1236-
return Err(NipcError::BadLayout);
1237-
}
1238-
}
1239-
_ => unreachable!(),
1240-
}
1241-
}
12421221
let label_count = match checked_u16(labels.len()) {
12431222
Ok(v) => v,
12441223
Err(err) => {

0 commit comments

Comments
 (0)