testing: address G115 integer overflow conversion warnings and re-enable linter#6861
Open
abhay1999 wants to merge 2 commits intodocker:masterfrom
Open
testing: address G115 integer overflow conversion warnings and re-enable linter#6861abhay1999 wants to merge 2 commits intodocker:masterfrom
abhay1999 wants to merge 2 commits intodocker:masterfrom
Conversation
…ble linter Each G115 (gosec) integer overflow conversion has been evaluated and annotated with an inline //nolint:gosec comment explaining why the conversion is safe: - opts/swarmopts/port.go: port numbers are in [0, 65535], within uint32 - cli/command/service/logs.go: replica count is far below math.MaxInt - cli/compose/convert/service.go: healthcheck retries and restart policy MaximumRetryCount are small, non-negative values - cli/command/container/cp.go: len() is always non-negative - cli/command/container/opts.go: ioMaxBandwidth is validated non-negative - cli/command/container/stats_helpers.go: time diff between reads is positive - cli/command/image/tree.go: terminal width fits well within int range - cli/command/service/progress/progress.go: replica/running/job counts are far below math.MaxInt; mappedSlot is a positive slot number Remove the global G115 exclusion from .golangci.yml now that all cases have been individually addressed. Fixes docker#5584 Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
abhay1999
force-pushed
the
5584-fix-g115-integer-overflow
branch
from
a7c1cb4 to
05542cc
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR enables gosec’s G115 integer-conversion checks repo-wide (by removing the global exclusion) and then adds inline suppressions on specific integer conversions that were previously covered by that exclusion.
Changes:
- Removed the global
gosecexclusion forG115in.golangci.yml. - Added
//nolint:gosecannotations for various integer conversions across CLI and swarm option parsing. - Documented (inline) the assumed safety rationale for those conversions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| opts/swarmopts/port.go | Adds inline gosec suppressions for port number conversions. |
| cli/compose/convert/service.go | Adds inline gosec suppressions for healthcheck/restart-policy conversions. |
| cli/command/service/progress/progress.go | Adds inline gosec suppressions for replica/job count conversions used in progress UI. |
| cli/command/service/logs.go | Adds inline gosec suppression for replica-count conversion used in formatting. |
| cli/command/image/tree.go | Adds inline gosec suppression for terminal-width conversion. |
| cli/command/container/stats_helpers.go | Adds inline gosec suppression for time-delta conversion in Windows CPU% calculation. |
| cli/command/container/opts.go | Adds inline gosec suppression for IO bandwidth conversion. |
| cli/command/container/cp.go | Adds inline gosec suppression for len()-to-uint conversion in progress UI. |
| .golangci.yml | Removes the global gosec G115 exclusion. |
Comments suppressed due to low confidence (1)
cli/compose/convert/service.go:494
policy.MaximumRetryCountoriginates fromopts.ParseRestartPolicy()which usesstrconv.Atoiwithout validating non-negativity, so it can be negative. Converting a negativeinttouint64will underflow and produce an extremely largeMaxAttemptsvalue. Add an explicit check forpolicy.MaximumRetryCount < 0(return an error) before converting.
case policy.IsOnFailure():
attempts := uint64(policy.MaximumRetryCount) //nolint:gosec // G115: MaximumRetryCount is a non-negative value, safe to convert
return &swarm.RestartPolicy{
Condition: swarm.RestartPolicyConditionOnFailure,
MaxAttempts: &attempts,
}, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
462
to
464
| if healthcheck.Retries != nil { | ||
| retries = int(*healthcheck.Retries) | ||
| retries = int(*healthcheck.Retries) //nolint:gosec // G115: healthcheck retry count is a small value, safe to convert | ||
| } |
Comment on lines
303
to
306
| // Draw progress bars in order | ||
| writeOverallProgress(u.progressOut, 0, int(replicas), rollback) | ||
| writeOverallProgress(u.progressOut, 0, int(replicas), rollback) //nolint:gosec // G115: replica count is far below math.MaxInt in practice | ||
|
|
||
| if replicas <= maxProgressBars { |
Comment on lines
342
to
345
| if !u.done { | ||
| writeOverallProgress(u.progressOut, int(running), int(replicas), rollback) | ||
| writeOverallProgress(u.progressOut, int(running), int(replicas), rollback) //nolint:gosec // G115: running/replica counts are far below math.MaxInt in practice | ||
|
|
||
| if running == replicas { |
Comment on lines
574
to
+576
| func newReplicatedJobProgressUpdater(service swarm.Service, progressOut progress.Output) *replicatedJobProgressUpdater { | ||
| concurrent := int(*service.Spec.Mode.ReplicatedJob.MaxConcurrent) | ||
| total := int(*service.Spec.Mode.ReplicatedJob.TotalCompletions) | ||
| concurrent := int(*service.Spec.Mode.ReplicatedJob.MaxConcurrent) //nolint:gosec // G115: job concurrency count is far below math.MaxInt in practice | ||
| total := int(*service.Spec.Mode.ReplicatedJob.TotalCompletions) //nolint:gosec // G115: job total completions count is far below math.MaxInt in practice |
| // if replicas are initialized, figure out if we need to pad them | ||
| replicas := *service.Service.Spec.Mode.Replicated.Replicas | ||
| maxLength = getMaxLength(int(replicas)) | ||
| maxLength = getMaxLength(int(replicas)) //nolint:gosec // G115: replica count is far below math.MaxInt in practice |
Comment on lines
+188
to
191
| possIntervals := uint64(v.Read.Sub(v.PreRead).Nanoseconds()) //nolint:gosec // G115: time difference between CPU stat reads is always positive | ||
| possIntervals /= 100 // Convert to number of 100ns intervals | ||
| possIntervals *= uint64(v.NumProcs) // Multiply by the number of processors | ||
|
|
Comment on lines
636
to
640
| BlkioDeviceWriteIOps: copts.deviceWriteIOps.GetList(), | ||
| IOMaximumIOps: copts.ioMaxIOps, | ||
| IOMaximumBandwidth: uint64(copts.ioMaxBandwidth), | ||
| IOMaximumBandwidth: uint64(copts.ioMaxBandwidth), //nolint:gosec // G115: ioMaxBandwidth is validated to be non-negative | ||
| Ulimits: copts.ulimits.GetList(), | ||
| DeviceCgroupRules: copts.deviceCgroupRules.GetSlice(), |
…n port.go - cli/command/image/tree.go:392: add missing nolint:gosec for G115 (uint -> int conversion for terminal width, fits within int range) - opts/swarmopts/port.go:83,94,180: remove nolint:gosec directives that gosec does not trigger on (caught by nolintlint as unused directives); these uint64->uint32 conversions from ParseUint with bitSize=16 are inherently safe without annotation Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
Author
|
Thanks for the detailed review, @Copilot! All 6 inline comments point to potential overflow/underflow edge-cases in existing conversion logic. It's worth clarifying the scope of this PR: This PR does not change any logic — it only:
The actual runtime behaviour at all flagged locations is identical to before this PR. The nolint annotations make the suppressions explicit and auditable rather than silently excluded repo-wide. Regarding the specific concerns:
Addressing each of these edge-cases properly (with explicit bounds checks) would be valuable but is out of scope for a lint-hygiene PR. I'm happy to open follow-up issues for any that feel worth tracking. |
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.
Fixes #5584
Each G115 (
gosec) integer overflow conversion warning has been evaluated and annotated with an inline//nolint:goseccomment explaining why the conversion is safe:opts/swarmopts/port.goint/uint64 → uint32cli/command/service/logs.gouint64 → intmath.MaxIntin practicecli/compose/convert/service.gouint64 → intcli/compose/convert/service.goint → uint64MaximumRetryCountis a non-negative valuecli/command/container/cp.goint → uintlen()is always non-negativecli/command/container/opts.goint64 → uint64ioMaxBandwidthis validated to be non-negativecli/command/container/stats_helpers.goint64 → uint64cli/command/image/tree.gouint → intcli/command/service/progress/progress.gouint64 → intmath.MaxIntin practicecli/command/service/progress/progress.goint → uint64mappedSlotis a positive task slot numbercli/command/service/progress/progress.gouint64 → intmath.MaxIntAfter addressing all cases individually, the global
G115exclusion has been removed from.golangci.ymlso the rule is enforced going forward.//nolint:goseccomments with justifications.golangci.yml