cache: don't hand-quote cpuset.CPUSet.String()'s. - #775
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated cpuset unmarshaling logic manually strips quotes instead of JSON-decoding the string, which can reject valid JSON inputs (e.g., whitespace/escapes) and is inconsistent with the new marshal behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens and standardizes how cpuset.CPUSet policy entries are encoded/decoded in the resource manager cache, addressing a CodeQL concern and adding input validation before parsing.
Changes:
- Replace manual quoting of
cpuset.CPUSet.String()withjson.Marshal(...)for cache storage. - Add basic boundary/format checks before attempting to unmarshal
cpuset.CPUSetentries.
File summaries
| File | Description |
|---|---|
| pkg/resmgr/cache/cache.go | Adjusts cpuset policy entry marshaling/unmarshaling behavior and adds validation on input data. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ddd8581 to
6576ac7
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes appear correct and reduce risk (no manual JSON quoting/slicing), with only minor optional improvements noted in review comments.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
askervin
left a comment
askervin
left a comment
There was a problem hiding this comment.
Yet there are good chances we'll never see the error from cpuset.Parse(), I still think copilot's suggestion on wrapping it in cacheError is a good idea. It would also be consistent with how cpuset.Parse() error is handled below when unmarshalling into map[string]cpuset.CPUSet.
JSON marshal cpuset.CPUSet.String()'s instead of hand-quoting to get rid of a CodeQL alert. Also, instead of cpuset.Parsing data[1:len(data)-1], json.Unmarshal to a string then parse it. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
6576ac7 to
1cd83ba
Compare
@askervin True, and it is also then more consistent with any previous string unmarshalling errors. Updated accordingly. |
JSON marshal cpuset.CPUSet.String()'s instead of hand-quoting to get rid of a CodeQL alert. Also, do boundary checks on the data before trying to unmarshal.