Skip to content

Commit a263a9b

Browse files
ktsaouktsaou
committed
Fix Go lookup code scanning alerts
1 parent 879c521 commit a263a9b

4 files changed

Lines changed: 67 additions & 123 deletions

File tree

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

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,15 @@ Refined recommendation:
357357
- Extracted cgroups lookup label-table writing into one local helper per language.
358358
- The cgroups lookup change intentionally did not change item offsets, label table layout, string storage order, directory entries, response finishing, or packed-data compaction.
359359
- Label source validation remains in the existing layout calculation path before label storage is written.
360+
- Investigated the failing GitHub Actions check `Go Static Analysis (src/go)` on commit `879c521`.
361+
- The failing job reported that `gosec` found issues and uploaded SARIF; GitHub code scanning exposed five open `G115` alerts:
362+
- alerts `7574`, `7575`, and `7576` at `pkg/netipc/protocol/lookup.go:1192`.
363+
- alerts `7577` and `7578` at `pkg/netipc/protocol/lookup.go:757`.
364+
- all five were `integer overflow conversion int -> uint64` from helper-call casts in the Go lookup codec.
365+
- Fixed the Go code-scanning findings by changing the Go lookup semantic helper length/count parameters to `int`, removing the introduced `int -> uint64` casts instead of suppressing `G115`.
366+
- Continued the apps lookup builder complexity target:
367+
- reused the extracted lookup label writer from both cgroups and apps lookup builders in Go, C, and Rust.
368+
- kept item offsets, label-table layout, string storage order, directory entries, response finishing, and packed-data compaction unchanged.
360369

361370
## Validation
362371

@@ -458,6 +467,41 @@ Implementation validation:
458467
- `bash .agents/sow/audit.sh` passed and reported SOW initialization complete and clean.
459468
- `git diff --check` passed.
460469
- Sensitive-data scan across the touched SOW and codec files returned no matches.
470+
- CI/code-scanning repair and apps lookup builder validation:
471+
- GitHub Actions failing check inspected:
472+
- run `26915313092`, job `79403453980`, check `Go Static Analysis (src/go)`.
473+
- root cause: `gosec` exited with status 1 after uploading SARIF.
474+
- GitHub code scanning showed five open `G115` alerts on `pkg/netipc/protocol/lookup.go`.
475+
- Local reproduction of the failing Go static-analysis job passed in `src/go`:
476+
- `go test ./...` passed.
477+
- `go vet ./...` passed.
478+
- `staticcheck ./...` passed.
479+
- `govulncheck ./...` passed with no vulnerabilities.
480+
- `gosec -quiet -fmt sarif -out /tmp/plugin-ipc-gosec-src-go.sarif -exclude=G404 ./...` passed with status `0`.
481+
- `cargo fmt --manifest-path src/crates/netipc/Cargo.toml` passed.
482+
- `cargo test --manifest-path src/crates/netipc/Cargo.toml` passed: 329 Rust unit tests passed, plus bin/doc test targets.
483+
- `cmake --build build` passed.
484+
- `/usr/bin/ctest --test-dir build --output-on-failure -R protocol` passed: 4/4 protocol tests.
485+
- `bash tests/interop_codec.sh` passed after CTest, serially:
486+
- Rust decoded C output: 89 passed, 0 failed.
487+
- Go decoded C output: 90 passed, 0 failed.
488+
- C decoded Rust output: 101 passed, 0 failed.
489+
- Go decoded Rust output: 90 passed, 0 failed.
490+
- C decoded Go output: 101 passed, 0 failed.
491+
- Rust decoded Go output: 89 passed, 0 failed.
492+
- C, Rust, and Go generated byte-identical protocol fixture files, including all apps lookup response variants.
493+
- Lizard focused lookup-code comparison for the apps lookup builder target:
494+
- Go `AppsLookupBuilder.Add`: CCN 39 before, 28 after.
495+
- C `nipc_apps_lookup_builder_add`: CCN 31 before, 29 after.
496+
- Rust `AppsLookupBuilder::add`: CCN 27 before, below the CCN 20 warning threshold after.
497+
- `codacy-analysis analyze --output-format json` passed:
498+
- Checkov: 0 issues.
499+
- Opengrep/Semgrep: 0 issues.
500+
- Trivy: 0 issues.
501+
- cppcheck: 0 issues.
502+
- ShellCheck: 0 issues.
503+
- Spectral: 0 issues.
504+
- total: 0 issues, 0 errors.
461505
- Cgroups lookup complexity validation:
462506
- `gofmt -w src/go/pkg/netipc/protocol/lookup.go` passed.
463507
- `cargo fmt --manifest-path src/crates/netipc/Cargo.toml` passed.
@@ -503,7 +547,7 @@ Sensitive data gate:
503547

504548
## Outcome
505549

506-
Raw cache, Go typed-facade, apps lookup, and cgroups lookup remediation targets are complete; the overall maintainability SOW remains in progress pending the next useful target or closure decision.
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.
507551

508552
## Lessons Extracted
509553

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

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,7 @@ impl<'a> CgroupsLookupBuilder<'a> {
853853
item[name_offset + name.len()] = 0;
854854
if !labels.is_empty() {
855855
item[fixed_end..table_start].fill(0);
856-
item_size = write_cgroups_lookup_labels(item, table_start, table_bytes, labels)?;
856+
item_size = write_lookup_labels(item, table_start, table_bytes, labels)?;
857857
}
858858
let dir_offset = (self.item_count as usize)
859859
.checked_mul(LOOKUP_DIR_ENTRY_SIZE)
@@ -887,7 +887,7 @@ impl<'a> CgroupsLookupBuilder<'a> {
887887
}
888888
}
889889

890-
fn write_cgroups_lookup_labels(
890+
fn write_lookup_labels(
891891
item: &mut [u8],
892892
table_start: usize,
893893
table_bytes: usize,
@@ -1308,35 +1308,7 @@ impl<'a> AppsLookupBuilder<'a> {
13081308
item[name_offset + cgroup_name.len()] = 0;
13091309
if !labels.is_empty() {
13101310
item[fixed_end..table_start].fill(0);
1311-
let mut next = table_start
1312-
.checked_add(table_bytes)
1313-
.ok_or(NipcError::Overflow)?;
1314-
for (i, (key, value)) in labels.iter().enumerate() {
1315-
let entry_offset = i
1316-
.checked_mul(LOOKUP_LABEL_ENTRY_SIZE)
1317-
.ok_or(NipcError::Overflow)?;
1318-
let entry = table_start
1319-
.checked_add(entry_offset)
1320-
.ok_or(NipcError::Overflow)?;
1321-
let value_offset = next
1322-
.checked_add(key.len())
1323-
.and_then(|v| v.checked_add(1))
1324-
.ok_or(NipcError::Overflow)?;
1325-
put_u32(item, entry, checked_u32(next)?);
1326-
put_u32(item, entry + 4, checked_u32(key.len())?);
1327-
put_u32(item, entry + 8, checked_u32(value_offset)?);
1328-
put_u32(item, entry + 12, checked_u32(value.len())?);
1329-
item[next..next + key.len()].copy_from_slice(key);
1330-
item[next + key.len()] = 0;
1331-
next = value_offset;
1332-
item[next..next + value.len()].copy_from_slice(value);
1333-
item[next + value.len()] = 0;
1334-
next = next
1335-
.checked_add(value.len())
1336-
.and_then(|v| v.checked_add(1))
1337-
.ok_or(NipcError::Overflow)?;
1338-
}
1339-
item_size = next;
1311+
item_size = write_lookup_labels(item, table_start, table_bytes, labels)?;
13401312
}
13411313
let dir_offset = (self.item_count as usize)
13421314
.checked_mul(LOOKUP_DIR_ENTRY_SIZE)

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

Lines changed: 12 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func invalidSourceString(data []byte, requireNonEmpty bool) bool {
6363
return (requireNonEmpty && len(data) == 0) || bytes.IndexByte(data, 0) >= 0
6464
}
6565

66-
func validateAppsLookupSemantics(status, cgroupStatus, orchestrator uint16, ppid, uid uint32, starttime uint64, commLen, pathLen, nameLen, labelCount uint64) error {
66+
func validateAppsLookupSemantics(status, cgroupStatus, orchestrator uint16, ppid, uid uint32, starttime uint64, commLen, pathLen, nameLen, labelCount int) error {
6767
if status != PidLookupKnown && status != PidLookupUnknown {
6868
return ErrBadLayout
6969
}
@@ -105,7 +105,7 @@ func validateAppsLookupSemantics(status, cgroupStatus, orchestrator uint16, ppid
105105
return nil
106106
}
107107

108-
func validateCgroupsLookupSemantics(status, orchestrator uint16, pathLen, nameLen, labelCount uint64) error {
108+
func validateCgroupsLookupSemantics(status, orchestrator uint16, pathLen, nameLen, labelCount int) error {
109109
if status != CgroupLookupKnown && status != CgroupLookupUnknownRetryLater && status != CgroupLookupUnknownPermanent {
110110
return ErrBadLayout
111111
}
@@ -754,7 +754,7 @@ func decodeCgroupsLookupItem(item []byte) (*CgroupsLookupItemView, error) {
754754
if ne.Uint16(item[0:2]) != 1 || ne.Uint16(item[6:8]) != 0 || ne.Uint16(item[26:28]) != 0 {
755755
return nil, ErrBadLayout
756756
}
757-
if err := validateCgroupsLookupSemantics(status, orchestrator, uint64(pathLen), uint64(nameLen), uint64(labelCount)); err != nil {
757+
if err := validateCgroupsLookupSemantics(status, orchestrator, pathLen, nameLen, int(labelCount)); err != nil {
758758
return nil, err
759759
}
760760
path, pathEnd, err := lookupString(item, CgroupsLookupItemHdr, pathOff, pathLen)
@@ -816,7 +816,7 @@ func (b *CgroupsLookupBuilder) Add(status, orchestrator uint16, path, name []byt
816816
b.err = ErrOverflow
817817
return ErrOverflow
818818
}
819-
if err := validateCgroupsLookupSemantics(status, orchestrator, uint64(len(path)), uint64(len(name)), uint64(len(labels))); err != nil {
819+
if err := validateCgroupsLookupSemantics(status, orchestrator, len(path), len(name), len(labels)); err != nil {
820820
b.err = err
821821
return err
822822
}
@@ -913,7 +913,7 @@ func (b *CgroupsLookupBuilder) Add(status, orchestrator uint16, path, name []byt
913913
item[nameOff+len(name)] = 0
914914
if len(labels) > 0 {
915915
clear(item[fixedEnd:tableStart])
916-
next, err := writeCgroupsLookupLabels(item, tableStart, tableBytes, labels)
916+
next, err := writeLookupLabels(item, tableStart, tableBytes, labels)
917917
if err != nil {
918918
b.err = err
919919
return err
@@ -932,7 +932,7 @@ func (b *CgroupsLookupBuilder) Add(status, orchestrator uint16, path, name []byt
932932
return nil
933933
}
934934

935-
func writeCgroupsLookupLabels(item []byte, tableStart, tableBytes int, labels []struct{ Key, Value []byte }) (int, error) {
935+
func writeLookupLabels(item []byte, tableStart, tableBytes int, labels []struct{ Key, Value []byte }) (int, error) {
936936
next, ok := checkedAddInt(tableStart, tableBytes)
937937
if !ok {
938938
return 0, ErrOverflow
@@ -1189,7 +1189,7 @@ func decodeAppsLookupItem(item []byte) (*AppsLookupItemView, error) {
11891189
if ne.Uint16(item[0:2]) != 1 || ne.Uint32(item[20:24]) != 0 || ne.Uint16(item[58:60]) != 0 {
11901190
return nil, ErrBadLayout
11911191
}
1192-
if err := validateAppsLookupSemantics(status, cgroupStatus, orchestrator, ppid, uid, starttime, uint64(commLen), uint64(pathLen), uint64(nameLen), uint64(labelCount)); err != nil {
1192+
if err := validateAppsLookupSemantics(status, cgroupStatus, orchestrator, ppid, uid, starttime, commLen, pathLen, nameLen, int(labelCount)); err != nil {
11931193
return nil, err
11941194
}
11951195
comm, commEnd, err := lookupString(item, AppsLookupItemHdr, commOff, commLen)
@@ -1263,7 +1263,7 @@ func (b *AppsLookupBuilder) Add(status, cgroupStatus, orchestrator uint16, pid,
12631263
b.err = ErrOverflow
12641264
return ErrOverflow
12651265
}
1266-
if err := validateAppsLookupSemantics(status, cgroupStatus, orchestrator, ppid, uid, starttime, uint64(len(comm)), uint64(len(cgroupPath)), uint64(len(cgroupName)), uint64(len(labels))); err != nil {
1266+
if err := validateAppsLookupSemantics(status, cgroupStatus, orchestrator, ppid, uid, starttime, len(comm), len(cgroupPath), len(cgroupName), len(labels)); err != nil {
12671267
b.err = err
12681268
return err
12691269
}
@@ -1388,66 +1388,10 @@ func (b *AppsLookupBuilder) Add(status, cgroupStatus, orchestrator uint16, pid,
13881388
item[nameOff+len(cgroupName)] = 0
13891389
if len(labels) > 0 {
13901390
clear(item[fixedEnd:tableStart])
1391-
next, ok := checkedAddInt(tableStart, tableBytes)
1392-
if !ok {
1393-
return ErrOverflow
1394-
}
1395-
for i, label := range labels {
1396-
keyOff32, ok := checkedU32Int(next)
1397-
if !ok {
1398-
b.err = ErrOverflow
1399-
return ErrOverflow
1400-
}
1401-
keyLen32, ok := checkedU32Int(len(label.Key))
1402-
if !ok {
1403-
b.err = ErrOverflow
1404-
return ErrOverflow
1405-
}
1406-
valueOff, ok := checkedAddInt(next, len(label.Key))
1407-
if ok {
1408-
valueOff, ok = checkedAddInt(valueOff, 1)
1409-
}
1410-
if !ok {
1411-
b.err = ErrOverflow
1412-
return ErrOverflow
1413-
}
1414-
valueOff32, ok := checkedU32Int(valueOff)
1415-
if !ok {
1416-
b.err = ErrOverflow
1417-
return ErrOverflow
1418-
}
1419-
valueLen32, ok := checkedU32Int(len(label.Value))
1420-
if !ok {
1421-
b.err = ErrOverflow
1422-
return ErrOverflow
1423-
}
1424-
entryRel, ok := checkedMulInt(i, LookupLabelEntrySize)
1425-
if !ok {
1426-
b.err = ErrOverflow
1427-
return ErrOverflow
1428-
}
1429-
entry, ok := checkedAddInt(tableStart, entryRel)
1430-
if !ok {
1431-
b.err = ErrOverflow
1432-
return ErrOverflow
1433-
}
1434-
ne.PutUint32(item[entry:entry+4], keyOff32)
1435-
ne.PutUint32(item[entry+4:entry+8], keyLen32)
1436-
ne.PutUint32(item[entry+8:entry+12], valueOff32)
1437-
ne.PutUint32(item[entry+12:entry+16], valueLen32)
1438-
copy(item[next:], label.Key)
1439-
item[next+len(label.Key)] = 0
1440-
next = valueOff
1441-
copy(item[next:], label.Value)
1442-
item[next+len(label.Value)] = 0
1443-
next, ok = checkedAddInt(next, len(label.Value))
1444-
if ok {
1445-
next, ok = checkedAddInt(next, 1)
1446-
}
1447-
if !ok {
1448-
b.err = ErrOverflow
1449-
return ErrOverflow
1450-
}
1391+
next, err := writeLookupLabels(item, tableStart, tableBytes, labels)
1392+
if err != nil {
1393+
b.err = err
1394+
return err
14511395
}
14521396
itemSize = next
14531397
}

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

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,11 +1148,11 @@ static nipc_error_t lookup_label_at(const uint8_t *item,
11481148
&out->value, &ignored);
11491149
}
11501150

1151-
static void cgroups_lookup_write_labels(uint8_t *item,
1152-
size_t table_start,
1153-
size_t table_bytes,
1154-
const nipc_lookup_label_view_t *labels,
1155-
uint16_t label_count)
1151+
static void lookup_write_labels(uint8_t *item,
1152+
size_t table_start,
1153+
size_t table_bytes,
1154+
const nipc_lookup_label_view_t *labels,
1155+
uint16_t label_count)
11561156
{
11571157
size_t next = table_start + table_bytes;
11581158
for (uint32_t i = 0; i < label_count; i++) {
@@ -1800,7 +1800,7 @@ nipc_error_t nipc_cgroups_lookup_builder_add(
18001800
if (label_count > 0) {
18011801
if (table_start > fixed_end)
18021802
memset(item + fixed_end, 0, table_start - fixed_end);
1803-
cgroups_lookup_write_labels(item, table_start, table_bytes, labels, label_count);
1803+
lookup_write_labels(item, table_start, table_bytes, labels, label_count);
18041804
}
18051805

18061806
nipc_lookup_dir_entry_t dir_entry = {
@@ -2162,23 +2162,7 @@ nipc_error_t nipc_apps_lookup_builder_add(
21622162
if (label_count > 0) {
21632163
if (table_start > fixed_end)
21642164
memset(item + fixed_end, 0, table_start - fixed_end);
2165-
size_t next = table_start + table_bytes;
2166-
for (uint32_t i = 0; i < label_count; i++) {
2167-
nipc_lookup_label_entry_t entry = {
2168-
.key_offset = (uint32_t)next,
2169-
.key_length = labels[i].key.len,
2170-
.value_offset = (uint32_t)(next + labels[i].key.len + 1u),
2171-
.value_length = labels[i].value.len,
2172-
};
2173-
memcpy(item + table_start + (size_t)i * NIPC_LOOKUP_LABEL_ENTRY_SIZE,
2174-
&entry, sizeof(entry));
2175-
memcpy(item + entry.key_offset, labels[i].key.ptr, labels[i].key.len);
2176-
item[entry.key_offset + labels[i].key.len] = '\0';
2177-
if (labels[i].value.len > 0)
2178-
memcpy(item + entry.value_offset, labels[i].value.ptr, labels[i].value.len);
2179-
item[entry.value_offset + labels[i].value.len] = '\0';
2180-
next = entry.value_offset + labels[i].value.len + 1u;
2181-
}
2165+
lookup_write_labels(item, table_start, table_bytes, labels, label_count);
21822166
}
21832167

21842168
nipc_lookup_dir_entry_t dir_entry = {

0 commit comments

Comments
 (0)