pkg/sysfs: replace interface{}-based sysfs I/O with typed generics#682
Open
kad wants to merge 1 commit into
Open
pkg/sysfs: replace interface{}-based sysfs I/O with typed generics#682kad wants to merge 1 commit into
kad wants to merge 1 commit into
Conversation
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>
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.
Comment on lines
+186
to
205
| // parseIntTo parses str as a signed integer of type T, using T's actual bit | ||
| // width. Named types (e.g. idset.ID) fall back to strconv.IntSize. | ||
| func parseIntTo[T signedInt](str string) (T, error) { | ||
| var zero T | ||
| var bits int | ||
| switch any(zero).(type) { | ||
| case int8: | ||
| bits = 8 | ||
| case int16: | ||
| bits = 16 | ||
| case int32: | ||
| bits = 32 | ||
| case int64: | ||
| bits = 64 | ||
| default: // int and any named type whose underlying type is int | ||
| bits = strconv.IntSize | ||
| } | ||
| v, err := strconv.ParseInt(str, 0, bits) | ||
| return T(v), err | ||
| } |
Comment on lines
+207
to
+226
| // parseUintTo parses str as an unsigned integer of type T, using T's actual bit | ||
| // width. Named types fall back to strconv.IntSize. | ||
| func parseUintTo[T unsignedInt](str string) (T, error) { | ||
| var zero T | ||
| var bits int | ||
| switch any(zero).(type) { | ||
| case uint8: | ||
| bits = 8 | ||
| case uint16: | ||
| bits = 16 | ||
| case uint32: | ||
| bits = 32 | ||
| case uint64: | ||
| bits = 64 | ||
| default: // uint and any named type whose underlying type is uint | ||
| bits = strconv.IntSize | ||
| } | ||
| v, err := strconv.ParseUint(str, 0, bits) | ||
| return T(v), err | ||
| } |
Comment on lines
+98
to
+107
| // Named types whose underlying type is int should parse at strconv.IntSize bits, | ||
| // not silently truncate into a smaller type. | ||
| t.Run("named_int_type", func(t *testing.T) { | ||
| type myID int | ||
| v, err := parseIntTo[myID]("100") | ||
| if err != nil || v != 100 { | ||
| t.Errorf("parseIntTo[myID](100) = %d, %v; want 100, nil", v, err) | ||
| } | ||
| }) | ||
| } |
Comment on lines
+153
to
+159
| t.Run("hex", func(t *testing.T) { | ||
| v, err := parseUintTo[uint32]("0xff") | ||
| if err != nil || v != 255 { | ||
| t.Errorf("parseUintTo[uint32](0xff) = %d, %v; want 255, nil", v, err) | ||
| } | ||
| }) | ||
| } |
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.
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