[Chore] Fix lint errors caused by casting int to int32 #2368
Conversation
| } | ||
|
|
||
| rayContainer.LivenessProbe = &corev1.Probe{ | ||
| InitialDelaySeconds: utils.DefaultLivenessProbeInitialDelaySeconds, | ||
| TimeoutSeconds: int32(probeTimeout), |
There was a problem hiding this comment.
The linter is not smart enough to determine whether casting probeTimeout (int) to int32 will cause an overflow, but it can determine if an overflow will occur when casting utils.DefaultLivenessProbeTimeoutSeconds or utils.DefaultHeadLivenessProbeTimeoutSeconds.
There was a problem hiding this comment.
I think it is not the fault of the linter. Because utils.DefaultLivenessProbeTimeoutSeconds is a constant so the linter can easily find it will not cause error if casted to int32. But for the int32(probeTimeout) case, the linter cannot determine if it will cause an overflow because it does not know whether probeTimeout is assigned with some other values in between.
There was a problem hiding this comment.
You can check securego/gosec#1212. The gosec community seems to consider it as a false positive because probeTimeout should be either utils.DefaultLivenessProbeTimeoutSeconds or utils.DefaultHeadLivenessProbeTimeoutSeconds which is smaller than MaxInt32 and should not cause overflow when casting.
| @@ -769,21 +769,20 @@ func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv | |||
| if worker.NumOfHosts <= 0 { | |||
| worker.NumOfHosts = 1 | |||
| } | |||
| numExpectedPods := workerReplicas * worker.NumOfHosts | |||
| numExpectedPods := int(workerReplicas * worker.NumOfHosts) | |||
There was a problem hiding this comment.
Converting int32 to int will not cause overflow. Therefore, I decide to convert int32 to int instead of converting int to int32.
There was a problem hiding this comment.
The code is doing the opposite, it converts to int instead of int32?
There was a problem hiding this comment.
Oh yeah, it is a typo in my comment.
.golangci.yml
Outdated
| @@ -4,6 +4,9 @@ linters-settings: | |||
| gosec: | |||
| excludes: | |||
| - G601 | |||
| # G115 is reporting false positives in golangci-lint v1.60.3. | |||
There was a problem hiding this comment.
G115 is reporting false negatives, and the gosec community still hasn't reached a final conclusion. Therefore, I’ve decided to disable it by default: securego/gosec#1212.
kevin85421
changed the title
kevin85421
changed the title
| @@ -251,7 +251,7 @@ func (r *RayServiceReconciler) calculateStatus(ctx context.Context, rayServiceIn | |||
| if numServeEndpoints > math.MaxInt32 { | |||
| return errstd.New("numServeEndpoints exceeds math.MaxInt32") | |||
| } | |||
| rayServiceInstance.Status.NumServeEndpoints = int32(numServeEndpoints) //nolint:gosec // Already checked in the previous line. | |||
| rayServiceInstance.Status.NumServeEndpoints = int32(numServeEndpoints) //nolint:gosec // This is a false positive from gosec. See https://github.com/securego/gosec/issues/1212 for more details. | |||
There was a problem hiding this comment.
I think this is not a false positive. The type of numServeEndPonits is int, so it may be 32 or 64 bits depending on the system. If it is 64 bits, then casting it to int32 will cause overflow. So this comment should not be changed.
There was a problem hiding this comment.
numServeEndpoints should be between 0 and MaxInt32, so converting it to int32 should not cause an overflow. Based on securego/gosec#1212, the gosec community considers it as a false positive.
| @@ -769,21 +769,20 @@ func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv | |||
| if worker.NumOfHosts <= 0 { | |||
| worker.NumOfHosts = 1 | |||
| } | |||
| numExpectedPods := workerReplicas * worker.NumOfHosts | |||
| numExpectedPods := int(workerReplicas * worker.NumOfHosts) | |||
There was a problem hiding this comment.
The code is doing the opposite, it converts to int instead of int32?
|
@MortalHappiness I merge this PR for now to unblock other PRs. If my replies don't make sense to you, we can discuss offline and maybe open a follow up PR instead. |
Why are these changes needed?
It may cause overflow when casting
inttoint32, andgolangci-lintrelies ongosec's rule G115 to check it. However, G115 causes some false positives, and thegoseccommunity is still working on the issue: securego/gosec#1212. We should remove//nolintuntil the false positive issue is solved.Open an issue to track the progress: #2369
Related issue number
Checks