Resolved g115 golanglint warning #5871
Open
+26
−21
Conversation
This file contains 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
Signed-off-by: Quang Hieu Le <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
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 <hanle.cs23@gmail.com>
thaJeztah
reviewed
Feb 25, 2025
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 |
Changing syntaxes for all previous nolint comment to narrow down false positive statement to only excluding G115 rules, avoid skipping other gosec validation. Signed-off-by: Quang Hieu Le <hanle.cs23@gmail.com>
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.
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