Resolved g115 golanglint warning #5871
Conversation
Signed-off-by: Quang Hieu Le <[email protected]>
Currently all known port number is in the range of int32, where the conversion within this file is usually a reduction (uint64 -> uint32), or expansion (int16 -> uint32), so the change of port number or related number overflowing is near 0. This makes nolint for gosec safe to append to the conversion within this file. Signed-off-by: Quang Hieu Le <[email protected]>
Replicas arg from Client does not currently have min/max number, but is accepted as uint64 (max 2^64 - 1), where int is a dynamic type (32 or 64 bit), assuming there is someone is willing to create 2^32 replicas, this library would be in a bigger trouble. Signed-off-by: Quang Hieu Le <[email protected]>
In both cases where int be 64 or 32 byte, this can be considered to be an expansion conversion (32 to 64 or 64 signed to 64 unsigned), which would not cause overflow Signed-off-by: Quang Hieu Le <[email protected]>
Currently the warning is there for conversion from int64 to uint64, regarding MemBytes flag (128M, 2G). While int64 allows negative value, the actual value that is passing for this flag cannot be negative due to the concept of negative memory does not exist (as far as the author know). At the same time, int64 and uint64 has the same byte size, so this conversion won't cause overflow. Signed-off-by: Quang Hieu Le <[email protected]>
Since len always return positive int, there's no need to concern about signed int, so the int conversion overflow warning from int -> uint can be ignored/skipped Signed-off-by: Quang Hieu Le <[email protected]>
Changing adjustColumn arg from accepting uint to int for more versatile usage, rather than the reverse, accepting uint and then convert back to int. This pushes the responsibility to realign, double check data prior to using this function to the client side, Signed-off-by: Quang Hieu Le <[email protected]>
All of this conversion is safe due to changing from unsigned to signed bits, but also including a refactor to changing the arg of a function from handling uint to int, and push the conversion to the caller funcion instead. Signed-off-by: Quang Hieu Le <[email protected]>
Previously the calculation within this function convert int64 to uint64 right off the bat, by adding an early exit while preRead is smaller or equal to 0, not only we don't need nolint but also avoid having to divide by 0 and further calculation with 0. Signed-off-by: Quang Hieu Le <[email protected]>
For policy.MaximumRetryCount, there exist another validation in container/hostconfig.go to validate for any invalid value smaller or equal than 0, making this safe to convert from int to uint64. Regarding healthcheck.Retries, the two case where this might fail is when docker is running in a 32bit system, and someone set retries to be 2^32 or larger, which the chance is extremely low. Signed-off-by: Quang Hieu Le <[email protected]>
Most of the issue here is either converting from uint64 to int or int64 to int. Since int is dependent on the system it's using, the only case where it gets overflow is when Docker is being used on a 32bits system, and when the number of process reaches over 2^32 - 1, or when the total process reaches beyond the previous mentioned limit. More validating can be checked before it reaches this point, but should not be implemented within this issue. Signed-off-by: Quang Hieu Le <[email protected]>
| fmt.Fprint(buf, aec.Column(uint(len(header)+1))) | ||
| fmt.Fprint(buf, aec.Column(uint(len(header)+1))) //nolint:gosec |
There was a problem hiding this comment.
(probably a bad example line to pick for my comment 😂)
It might be good to use gosec's #nosec comments for these; they allow for only this specific linter rule to be ignored, which can help prevent possible other future gosec-related linting warnings to still be detected.
Where possible, I also try to add a (short) comment to these, to describe why it's safe to ignore (doesn't have to be too much, but a short comment can help a future visitor of the code understand what we're ignoring, and why).
The format gosec defined for that is a bit non-standard, but like;
// #gosec <ISSUE> -- comment goes hereSo something like;
// #nosec G115 -- ignore "possible integer overflow conversion" (some explanation here)There was a problem hiding this comment.
Will update all the previous commit that I made! Thank you for letting me know about this gosec syntax!
There was a problem hiding this comment.
Thank you for working on this!
Yes, the #gosec comments are a bit odd; they also don't comply with Go's defined format for "machine-parseable" comments; a "normal" comment must start with // and a space (//<space>Some comment I wrote), and "machine-parseable" comments do not start with a space, but must be (e.g. for nolint); //nolint:<something> , or (if you want to add a comment to that) //nolint:<something> // My comment about the nolint).
There was a problem hiding this comment.
No problem!
It does seems odds, but be able to narrow it down to only excludes G115 warning was what I tried to search for at the beginning so it's perfect for this use cases!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5871 +/- ##
==========================================
- Coverage 59.30% 59.29% -0.01%
==========================================
Files 353 353
Lines 29694 29698 +4
==========================================
Hits 17609 17609
- Misses 11104 11108 +4
Partials 981 981 |
Closes #5584
- What I did
I verified all the G115 warning mentioned in issue #5584, regarding int conversion overflow. Most of it seems to be fine given the current human usage capability, and on one occasion I introduced an early exit in container/stats_helpers.go to both avoid unecessary int conversion, and to resolve the conversion warning. Details explaination for each resolution can be found on specific commits, separated by each files.
- How I did it
I traced the value prior to conversion from its root, to check for whether if a value is supposed to be the way it is, if it can be negative, or if it's realistic to keep it the way it is (which a brief explaination can be found on each commit). If there is an unnecessary conversion (start from int, convert to uint to align with a function arg type, and then convert back to int again), I would modify the function to accept int right off the bat and introduce a checking for negative and early exit after function call.
- How to verify it
All the lint resolution can be check using docker buildx bake lint shellcheck, and then run all the test using docker buildx bake test. This changes should not impact other functionality.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
From https://unsplash.com/@michaelsum1228