Skip to content

Commit

Permalink
Fix Integer Overflow Warnings
Browse files Browse the repository at this point in the history
that were identified by the new GolangCILint version.
  • Loading branch information
mpass99 committed Aug 21, 2024
1 parent 467829e commit d21852e
Show file tree
Hide file tree
Showing 12 changed files with 114 additions and 37 deletions.
2 changes: 2 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ linters:
- testpackage
- varnamelen
- wsl
# Deprecated
- execinquery
- exportloopref
- gomnd

linters-settings:
Expand Down
8 changes: 7 additions & 1 deletion cmd/poseidon/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"crypto/x509"
"errors"
"fmt"
"math"
"net"
"net/http"
"os"
Expand Down Expand Up @@ -136,6 +137,11 @@ func watchMemoryAndAlert(options config.Profiling) {
if options.MemoryInterval == 0 {
return
}
if options.MemoryInterval > uint(math.MaxInt64)/uint(time.Millisecond) {
log.WithField("interval", options.MemoryInterval).Error("Configured memory interval too big")
return

Check warning on line 142 in cmd/poseidon/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/poseidon/main.go#L140-L142

Added lines #L140 - L142 were not covered by tests
}
intervalDuration := time.Duration(options.MemoryInterval) * time.Millisecond //nolint:gosec // We check for an integer overflow right above.

Check warning on line 144 in cmd/poseidon/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/poseidon/main.go#L144

Added line #L144 was not covered by tests

var exceeded bool
for {
Expand Down Expand Up @@ -163,7 +169,7 @@ func watchMemoryAndAlert(options config.Profiling) {
}

select {
case <-time.After(time.Duration(options.MemoryInterval) * time.Millisecond):
case <-time.After(intervalDuration):

Check warning on line 172 in cmd/poseidon/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/poseidon/main.go#L172

Added line #L172 was not covered by tests
continue
case <-context.Background().Done():
return
Expand Down
4 changes: 2 additions & 2 deletions internal/environment/aws_environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ func (a *AWSEnvironment) CPULimit() uint {
}

// SetCPULimit is disabled as one can only set the memory limit with AWS Lambda.
func (a *AWSEnvironment) SetCPULimit(_ uint) {}
func (a *AWSEnvironment) SetCPULimit(_ uint) error { return nil }

Check warning on line 89 in internal/environment/aws_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/aws_environment.go#L89

Added line #L89 was not covered by tests

func (a *AWSEnvironment) MemoryLimit() uint {
const memorySizeOfDeployedLambdaFunction = 2048 // configured /deploy/aws/template.yaml
return memorySizeOfDeployedLambdaFunction
}

func (a *AWSEnvironment) SetMemoryLimit(_ uint) {
func (a *AWSEnvironment) SetMemoryLimit(_ uint) error {

Check warning on line 96 in internal/environment/aws_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/aws_environment.go#L96

Added line #L96 was not covered by tests
panic("not supported")
}

Expand Down
53 changes: 38 additions & 15 deletions internal/environment/nomad_environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"math"
"strconv"
"sync"
"time"
Expand Down Expand Up @@ -64,8 +65,12 @@ func NewNomadEnvironmentFromRequest(ctx context.Context,

// Set options according to request
environment.SetPrewarmingPoolSize(request.PrewarmingPoolSize)
environment.SetCPULimit(request.CPULimit)
environment.SetMemoryLimit(request.MemoryLimit)
if err = environment.SetCPULimit(request.CPULimit); err != nil {
return nil, err

Check warning on line 69 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L69

Added line #L69 was not covered by tests
}
if err = environment.SetMemoryLimit(request.MemoryLimit); err != nil {
return nil, err

Check warning on line 72 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L72

Added line #L72 was not covered by tests
}
environment.SetImage(request.Image)
environment.SetNetworkAccess(request.NetworkAccess, request.ExposedPorts)
return environment, nil
Expand Down Expand Up @@ -101,7 +106,7 @@ func (n *NomadEnvironment) SetPrewarmingPoolSize(count uint) {
if taskGroup.Meta == nil {
taskGroup.Meta = make(map[string]string)
}
taskGroup.Meta[nomad.ConfigMetaPoolSizeKey] = strconv.Itoa(int(count))
taskGroup.Meta[nomad.ConfigMetaPoolSizeKey] = strconv.FormatUint(uint64(count), 10)
}

func (n *NomadEnvironment) CPULimit() uint {
Expand All @@ -110,12 +115,17 @@ func (n *NomadEnvironment) CPULimit() uint {
return uint(*defaultTask.Resources.CPU)
}

func (n *NomadEnvironment) SetCPULimit(limit uint) {
func (n *NomadEnvironment) SetCPULimit(limit uint) error {
if limit > math.MaxInt32 {
return fmt.Errorf("limit too high: %w", util.ErrMaxNumberExceeded)

Check warning on line 120 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L120

Added line #L120 was not covered by tests
}

defaultTaskGroup := nomad.FindAndValidateDefaultTaskGroup(n.job)
defaultTask := nomad.FindAndValidateDefaultTask(defaultTaskGroup)

integerCPULimit := int(limit)
integerCPULimit := int(limit) //nolint:gosec // We check for an integer overflow right above.
defaultTask.Resources.CPU = &integerCPULimit
return nil
}

func (n *NomadEnvironment) MemoryLimit() uint {
Expand All @@ -128,12 +138,17 @@ func (n *NomadEnvironment) MemoryLimit() uint {
return 0
}

func (n *NomadEnvironment) SetMemoryLimit(limit uint) {
func (n *NomadEnvironment) SetMemoryLimit(limit uint) error {
if limit > math.MaxInt32 {
return fmt.Errorf("limit too high: %w", util.ErrMaxNumberExceeded)

Check warning on line 143 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L143

Added line #L143 was not covered by tests
}

defaultTaskGroup := nomad.FindAndValidateDefaultTaskGroup(n.job)
defaultTask := nomad.FindAndValidateDefaultTask(defaultTaskGroup)

integerMemoryMaxLimit := int(limit)
integerMemoryMaxLimit := int(limit) //nolint:gosec // We check for an integer overflow right above.
defaultTask.Resources.MemoryMaxMB = &integerMemoryMaxLimit
return nil
}

func (n *NomadEnvironment) Image() string {
Expand All @@ -160,7 +175,11 @@ func (n *NomadEnvironment) NetworkAccess() (allowed bool, ports []uint16) {
if len(defaultTaskGroup.Networks) > 0 {
networkResource := defaultTaskGroup.Networks[0]
for _, port := range networkResource.DynamicPorts {
ports = append(ports, uint16(port.To))
if port.To > math.MaxUint16 {
log.WithField("port", port).Error("port number exceeds range")

Check warning on line 179 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L179

Added line #L179 was not covered by tests
} else {
ports = append(ports, uint16(port.To)) //nolint:gosec // We check for an integer overflow right above.
}
}
}
return allowed, ports
Expand Down Expand Up @@ -240,15 +259,15 @@ func (n *NomadEnvironment) Delete(reason runner.DestroyReason) error {
}

func (n *NomadEnvironment) ApplyPrewarmingPoolSize() error {
required := int(n.PrewarmingPoolSize()) - int(n.idleRunners.Length())

if required < 0 {
if n.PrewarmingPoolSize() < n.idleRunners.Length() {
log.WithError(ErrScaleDown).
WithField(dto.KeyEnvironmentID, n.ID().ToString()).
WithField("offset", -required).Info("Too many idle runner")
WithField("pool size", n.PrewarmingPoolSize()).
WithField("idle runners", n.idleRunners.Length()).
Info("Too many idle runner")

Check warning on line 267 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L265-L267

Added lines #L265 - L267 were not covered by tests
return nil
}
return n.createRunners(uint(required), true)
return n.createRunners(n.PrewarmingPoolSize()-n.idleRunners.Length(), true)
}

func (n *NomadEnvironment) Sample() (runner.Runner, bool) {
Expand Down Expand Up @@ -328,8 +347,12 @@ func (n *NomadEnvironment) DeepCopyJob() *nomadApi.Job {
func (n *NomadEnvironment) SetConfigFrom(environment runner.ExecutionEnvironment) {
n.SetID(environment.ID())
n.SetPrewarmingPoolSize(environment.PrewarmingPoolSize())
n.SetCPULimit(environment.CPULimit())
n.SetMemoryLimit(environment.MemoryLimit())
if err := n.SetCPULimit(environment.CPULimit()); err != nil {
log.WithError(err).Error("Failed to copy CPU Limit")

Check warning on line 351 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L351

Added line #L351 was not covered by tests
}
if err := n.SetMemoryLimit(environment.MemoryLimit()); err != nil {
log.WithError(err).Error("Failed to copy Memory Limit")

Check warning on line 354 in internal/environment/nomad_environment.go

View check run for this annotation

Codecov / codecov/patch

internal/environment/nomad_environment.go#L354

Added line #L354 was not covered by tests
}
n.SetImage(environment.Image())
n.SetNetworkAccess(environment.NetworkAccess())
}
Expand Down
7 changes: 6 additions & 1 deletion internal/nomad/command_execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io"
"math"
"strings"
"time"

Expand Down Expand Up @@ -167,7 +168,11 @@ func injectStartDebugMessage(command string, start uint, end int) string {
commandFields := strings.Fields(command)
if start < uint(len(commandFields)) {
commandFields = commandFields[start:]
end -= int(start)

if start > uint(math.MaxInt32)-uint(end) {
log.WithField("start", start).Error("passed start too big")

Check warning on line 173 in internal/nomad/command_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/nomad/command_execution.go#L173

Added line #L173 was not covered by tests
}
end -= int(start) //nolint:gosec // We check for an integer overflow right above.
}
if end >= 0 && end < len(commandFields) {
commandFields = commandFields[:end]
Expand Down
8 changes: 4 additions & 4 deletions internal/runner/execution_environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ type ReadableExecutionEnvironment interface {
type WriteableExecutionEnvironment interface {
SetID(id dto.EnvironmentID)
SetPrewarmingPoolSize(count uint)
SetCPULimit(limit uint)
SetMemoryLimit(limit uint)
SetCPULimit(limit uint) error
SetMemoryLimit(limit uint) error
SetImage(image string)
SetNetworkAccess(allow bool, ports []uint16)

Expand Down Expand Up @@ -81,8 +81,8 @@ type ExecutionEnvironment interface {
func monitorEnvironmentData(dataPoint *write.Point, e ExecutionEnvironment, eventType storage.EventType) {
if eventType == storage.Creation && e != nil {
dataPoint.AddTag("image", e.Image())
dataPoint.AddTag("cpu_limit", strconv.Itoa(int(e.CPULimit())))
dataPoint.AddTag("memory_limit", strconv.Itoa(int(e.MemoryLimit())))
dataPoint.AddTag("cpu_limit", strconv.FormatUint(uint64(e.CPULimit()), 10))
dataPoint.AddTag("memory_limit", strconv.FormatUint(uint64(e.MemoryLimit()), 10))
hasNetworkAccess, _ := e.NetworkAccess()
dataPoint.AddTag("network_access", strconv.FormatBool(hasNetworkAccess))
}
Expand Down
36 changes: 31 additions & 5 deletions internal/runner/execution_environment_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion internal/runner/nomad_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"math"
"strconv"
"sync"
"time"
Expand Down Expand Up @@ -151,6 +152,10 @@ func (m *NomadRunnerManager) checkPrewarmingPoolAlert(ctx context.Context, envir

prewarmingPoolThreshold := config.Config.Server.Alert.PrewarmingPoolThreshold
reloadTimeout := config.Config.Server.Alert.PrewarmingPoolReloadTimeout
if reloadTimeout > uint(math.MaxInt64)/uint(time.Second) {
log.WithField("timeout", reloadTimeout).Error("configured reload timeout too big")

Check warning on line 156 in internal/runner/nomad_manager.go

View check run for this annotation

Codecov / codecov/patch

internal/runner/nomad_manager.go#L156

Added line #L156 was not covered by tests
}
reloadTimeoutDuration := time.Duration(reloadTimeout) * time.Second //nolint:gosec // We check for an integer overflow right above.

if reloadTimeout == 0 || float64(environment.IdleRunnerCount())/float64(environment.PrewarmingPoolSize()) >= prewarmingPoolThreshold {
return
Expand All @@ -163,7 +168,7 @@ func (m *NomadRunnerManager) checkPrewarmingPoolAlert(ctx context.Context, envir
select {
case <-ctx.Done():
return
case <-time.After(time.Duration(reloadTimeout) * time.Second):
case <-time.After(reloadTimeoutDuration):
}

if float64(environment.IdleRunnerCount())/float64(environment.PrewarmingPoolSize()) >= prewarmingPoolThreshold {
Expand Down
4 changes: 2 additions & 2 deletions internal/runner/nomad_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,8 @@ func (s *MainTestSuite) TestNomadRunnerManager_Load() {
}

func (s *MainTestSuite) TestNomadRunnerManager_checkPrewarmingPoolAlert() {
timeout := uint(1)
config.Config.Server.Alert.PrewarmingPoolReloadTimeout = timeout
timeout := 1
config.Config.Server.Alert.PrewarmingPoolReloadTimeout = uint(timeout)
config.Config.Server.Alert.PrewarmingPoolThreshold = 0.5
environment := &ExecutionEnvironmentMock{}
environment.On("ID").Return(dto.EnvironmentID(tests.DefaultEnvironmentIDAsInteger))
Expand Down
1 change: 1 addition & 0 deletions pkg/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var (
// InitialWaitingDuration is the default initial duration of waiting after a failed time.
InitialWaitingDuration = time.Second
ErrRetryContextDone = errors.New("the retry context is done")
ErrMaxNumberExceeded = errors.New("the passed number is too big")
)

func retryExponential(ctx context.Context, sleep time.Duration, f func() error) func() error {
Expand Down
15 changes: 10 additions & 5 deletions tests/e2e/environments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"io"
"math"
"net/http"
"strings"
"testing"
Expand Down Expand Up @@ -357,15 +358,18 @@ func validateJob(t *testing.T, expected dto.ExecutionEnvironmentRequest) {
taskGroup := job.TaskGroups[0]
require.NotNil(t, taskGroup.Count)
// Poseidon might have already scaled the registered job up once.
prewarmingPoolSizeInt := int(expected.PrewarmingPoolSize)
require.Less(t, expected.PrewarmingPoolSize, uint(math.MaxInt32))
prewarmingPoolSizeInt := int(expected.PrewarmingPoolSize) //nolint:gosec // We check for an integer overflow right above.
taskGroupCount := *taskGroup.Count
assert.True(t, prewarmingPoolSizeInt == taskGroupCount || prewarmingPoolSizeInt+1 == taskGroupCount)
assertEqualValueIntPointer(t, int(expected.PrewarmingPoolSize), taskGroup.Count)
assertEqualValueIntPointer(t, prewarmingPoolSizeInt, taskGroup.Count)
require.Len(t, taskGroup.Tasks, 1)

task := taskGroup.Tasks[0]
assertEqualValueIntPointer(t, int(expected.CPULimit), task.Resources.CPU)
assertEqualValueIntPointer(t, int(expected.MemoryLimit), task.Resources.MemoryMaxMB)
require.Less(t, expected.CPULimit, uint(math.MaxInt32))
assertEqualValueIntPointer(t, int(expected.CPULimit), task.Resources.CPU) //nolint:gosec // We check for an integer overflow right above.
require.Less(t, expected.MemoryLimit, uint(math.MaxInt32))
assertEqualValueIntPointer(t, int(expected.MemoryLimit), task.Resources.MemoryMaxMB) //nolint:gosec // We check for an integer overflow right above.
assert.Equal(t, expected.Image, task.Config["image"])

if expected.NetworkAccess {
Expand All @@ -374,7 +378,8 @@ func validateJob(t *testing.T, expected dto.ExecutionEnvironmentRequest) {
network := taskGroup.Networks[0]
assert.Equal(t, len(expected.ExposedPorts), len(network.DynamicPorts))
for _, port := range network.DynamicPorts {
assert.Contains(t, expected.ExposedPorts, uint16(port.To))
require.Less(t, port.To, math.MaxUint16)
assert.Contains(t, expected.ExposedPorts, uint16(port.To)) //nolint:gosec // We check for an integer overflow right above.
}
} else {
assert.Equal(t, "none", task.Config["network_mode"])
Expand Down
6 changes: 5 additions & 1 deletion tests/e2e/websocket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ func (s *E2ETestSuite) TestMemoryMaxLimit_Nomad() {

stdout, stderr, _ := ExecuteNonInteractive(&s.Suite, tests.DefaultEnvironmentIDAsInteger, &dto.ExecutionRequest{
// This shell line tries to load maxMemoryLimit Bytes into the memory.
Command: "</dev/zero head -c " + strconv.Itoa(int(maxMemoryLimit)) + "MB | tail > /dev/null",
Command: "</dev/zero head -c " + strconv.FormatUint(uint64(maxMemoryLimit), 10) + "MB | tail > /dev/null",
}, nil)
s.Empty(stdout)
s.Contains(stderr, "Killed")
Expand Down Expand Up @@ -334,6 +334,10 @@ func (s *E2ETestSuite) ListTempDirectory(runnerID string) string {
break
}
}
if runningAllocStub == nil {
s.FailNow("No valid allocation found")
return ""
}
alloc, _, err := nomadClient.Allocations().Info(runningAllocStub.ID, nil)
s.Require().NoError(err)

Expand Down

0 comments on commit d21852e

Please sign in to comment.