pkg/sysfs: replace interface{}-based sysfs I/O with typed generics#682
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors sysfs I/O helpers in pkg/sysfs to replace the previous interface{}-based read/write layer with typed generic functions, aiming to prevent integer truncation bugs by enforcing correct range checks at parse time.
Changes:
- Introduces typed generic sysfs helpers (
readSysfsRaw/String/Int/Uint/IDSet/IntList/EPPandwriteSysfsRaw/Int/Uint) and removes the oldreadSysfsEntry/writeSysfsEntryinterface-based API. - Updates sysfs consumers (notably
system.go) to use the new typed helpers. - Adds a new
utils_test.gowith coverage for the new parsing and sysfs helper behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/sysfs/utils.go | Replaces untyped sysfs I/O utilities with typed generic read/write and parsing helpers. |
| pkg/sysfs/utils_test.go | Adds unit tests for the new generic parsing and sysfs read/write utilities. |
| pkg/sysfs/system.go | Migrates sysfs reads/writes to the new typed helper APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kad Should we use the copilot suggested conversion overflow check for named types and remove the unused parseUintSlice[T] ? Otherwise looks good to me. |
I've got better idea, to use reflect to get actual size of any type, so it should be simplier now. Please check newer revision. |
CodeQL flagged integer conversion bugs here (PR containers#676 was the autofix attempt). The real problem: readSysfsEntry/writeSysfsEntry accepted interface{} and every integer parser called strconv.ParseInt with bitSize=0, meaning 64-bit, then cast the result down -- so parsing "300" into int8 silently gave you 44. Patching the casts doesn't help; the issue is the parse site itself. Replaced the whole interface{}-based layer with typed generic functions: readSysfsRaw, readSysfsString, readSysfsInt[T], readSysfsUint[T], readSysfsIDSet, readSysfsIntList[T], readSysfsEPP on the read side, and writeSysfsRaw, writeSysfsInt[T], writeSysfsUint[T] for writes. parseIntTo[T] and parseUintTo[T] use a type switch to get T's actual bit width and pass it straight to strconv.ParseInt/ParseUint. Out-of-range values now error. The unsafe import is gone. A few other things were broken in the old code: - parseValueList `break`-ed on empty tokens, stopping the whole parse instead of just skipping the empty slot - parseIDSet split on "-" without a limit, so "0-7" got three pieces when the separator also happened to be "-"; fixed with SplitN(..., 2) - formatValueList returned "" instead of the string it built -- removed along with formatValue and other dead helpers All 27 call sites in system.go updated. Added utils_test.go covering all new utility functions. Replaces: containers#676 Signed-off-by: Alexander Kanevskiy <alexander.kanevskiy@intel.com>
askervin
left a comment
askervin
left a comment
There was a problem hiding this comment.
LGTM.
Haven't seen this causing any issues in e2e tests either, only unrelated instability. I think this is ok to merge.
CodeQL flagged integer conversion bugs here (PR #676 was the autofix attempt). The real problem: readSysfsEntry/writeSysfsEntry accepted interface{} and every integer parser called strconv.ParseInt with bitSize=0, meaning 64-bit, then cast the result down -- so parsing "300" into int8 silently gave you 44. Patching the casts doesn't help; the issue is the parse site itself.
Replaced the whole interface{}-based layer with typed generic functions: readSysfsRaw, readSysfsString, readSysfsInt[T], readSysfsUint[T], readSysfsIDSet, readSysfsIntList[T], readSysfsEPP on the read side, and writeSysfsRaw, writeSysfsInt[T], writeSysfsUint[T] for writes.
parseIntTo[T] and parseUintTo[T] use a type switch to get T's actual bit width and pass it straight to strconv.ParseInt/ParseUint. Out-of-range values now error. The unsafe import is gone.
A few other things were broken in the old code:
break-ed on empty tokens, stopping the whole parse instead of just skipping the empty slotAll 27 call sites in system.go updated. Added utils_test.go covering all new utility functions.
Replaces: #676