Skip to content

Commit

Permalink
Fix integer overflow lint errors (#4227)
Browse files Browse the repository at this point in the history
Fixed through a mixture of type changes, and ignoring the linter in
places where I was confident that we would never hit an overflow in
practice.
  • Loading branch information
dmjb authored Aug 21, 2024
1 parent a7d9023 commit 7a0da06
Show file tree
Hide file tree
Showing 12 changed files with 41 additions and 16 deletions.
4 changes: 2 additions & 2 deletions cmd/cli/app/provider/provider_enroll.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func enrollUsingOAuth2Flow(
resp, err := oauthClient.GetAuthorizationURL(ctx, &minderv1.GetAuthorizationURLRequest{
Context: &minderv1.Context{Provider: &providerName, Project: &project},
Cli: true,
Port: int32(port),
Port: port,
Owner: &owner,
Config: providerConfig,
ProviderClass: providerClass,
Expand All @@ -240,7 +240,7 @@ func enrollUsingOAuth2Flow(

done := make(chan bool)

go callBackServer(oAuthCallbackCtx, cmd, project, port, done, oauthClient, openTime, resp.GetState())
go callBackServer(oAuthCallbackCtx, cmd, project, int(port), done, oauthClient, openTime, resp.GetState())

success := <-done

Expand Down
2 changes: 1 addition & 1 deletion cmd/server/app/history_purge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestRecordSize(t *testing.T) {
},
)

require.Equal(t, 96, int(size))
require.Equal(t, uintptr(96), size)
}

func TestPurgeLoop(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion database/query/eval_history.sql
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ SELECT s.id::uuid AS evaluation_id,
ORDER BY
CASE WHEN sqlc.narg(next)::timestamp without time zone IS NULL THEN s.evaluation_time END ASC,
CASE WHEN sqlc.narg(prev)::timestamp without time zone IS NULL THEN s.evaluation_time END DESC
LIMIT sqlc.arg(size)::integer;
LIMIT sqlc.arg(size)::bigint;

-- name: ListEvaluationHistoryStaleRecords :many
SELECT s.evaluation_time,
Expand Down
11 changes: 9 additions & 2 deletions internal/crypto/nonce.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"crypto/rand"
"encoding/base64"
"encoding/binary"
"math"
"time"
)

Expand Down Expand Up @@ -49,9 +50,15 @@ func IsNonceValid(nonce string, noncePeriod int64) (bool, error) {
return false, nil
}

storedTimestamp := int64(binary.BigEndian.Uint64(nonceBytes[:8]))
storedTimestamp := binary.BigEndian.Uint64(nonceBytes[:8])
if storedTimestamp > math.MaxInt64 {
// stored timestamp is before Unix epoch -> bad value
return false, nil
}
currentTimestamp := time.Now().Unix()
timeDiff := currentTimestamp - storedTimestamp
// Checked explicitly for overflow in stored timestamp
// nolint: gosec
timeDiff := currentTimestamp - int64(storedTimestamp)

if timeDiff > noncePeriod { // 5 minutes = 300 seconds
return false, nil
Expand Down
6 changes: 4 additions & 2 deletions internal/db/embedded/testdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,13 @@ func GetFakeStore() (db.Store, CancelFunc, error) {
return db.NewStore(sqlDB), cancel, nil
}

func pickUnusedPort() (int, error) {
func pickUnusedPort() (int32, error) {
l, err := net.Listen("tcp", "localhost:0")
if err != nil {
return 0, err
}
defer l.Close()
return l.Addr().(*net.TCPAddr).Port, nil
// largest TCP port is 2^16, overflow should not happen
// nolint: gosec
return int32(l.Addr().(*net.TCPAddr).Port), nil
}
4 changes: 2 additions & 2 deletions internal/db/entity_execution_lock.sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ func TestQueries_LockIfThresholdNotExceeded(t *testing.T) {

wg.Wait()

assert.Equal(t, int32(concurrentCalls-1), queueCount.Load(), "expected all but one to be queued")
assert.Equal(t, int32(1), effectiveFlush.Load(), "expected only one flush to be queued")
assert.Equal(t, concurrentCalls-1, int(queueCount.Load()), "expected all but one to be queued")
assert.Equal(t, 1, int(effectiveFlush.Load()), "expected only one flush to be queued")

t.Log("sleeping for threshold")
time.Sleep(time.Duration(threshold) * time.Second)
Expand Down
4 changes: 2 additions & 2 deletions internal/db/eval_history.sql.go

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

1 change: 1 addition & 0 deletions internal/engine/ingester/diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ func ingestFileForFullDiff(filename, patch, patchUrl string) (*pbinternal.PrCont
result = append(result, &pbinternal.PrContents_File_Line{
Content: line[1:],
// see the use of strconv.ParseInt above: this is a safe downcast
// nolint: gosec
LineNumber: int32(currentLineNumber),
})

Expand Down
2 changes: 1 addition & 1 deletion internal/history/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (_ *evaluationHistoryService) ListEvaluationHistory(
filter ListEvaluationFilter,
) (*ListEvaluationHistoryResult, error) {
params := db.ListEvaluationHistoryParams{
Size: int32(size),
Size: int64(size),
}

if err := toSQLCursor(cursor, &params); err != nil {
Expand Down
9 changes: 8 additions & 1 deletion internal/util/cli/styles.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package cli

import (
"math"
"os"

"github.com/charmbracelet/lipgloss"
Expand Down Expand Up @@ -78,7 +79,13 @@ var (

func init() {
// Get the terminal width, if available, and set widths based on terminal width
w, _, err := term.GetSize(int(os.Stdout.Fd()))
fd := os.Stdout.Fd()
if fd > math.MaxInt32 {
return
}
// checked for overflow explicitly
// nolint: gosec
w, _, err := term.GetSize(int(fd))
if err == nil {
DefaultBannerWidth = w
}
Expand Down
6 changes: 6 additions & 0 deletions internal/util/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"io"
"io/fs"
"math"
"net/http"
"net/url"
"os"
Expand Down Expand Up @@ -469,6 +470,11 @@ func Int32FromString(v string) (int32, error) {
if err != nil {
return 0, fmt.Errorf("error converting string to int: %w", err)
}
if asInt32 > math.MaxInt32 || asInt32 < math.MinInt32 {
return 0, fmt.Errorf("integer %d cannot fit into int32", asInt32)
}
// already validated overflow
// nolint:gosec
return int32(asInt32), nil
}

Expand Down
6 changes: 4 additions & 2 deletions internal/util/rand/random.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,15 @@ func RandomFrom[T any](choices []T, seed int64) T {
// Marking a nosec here because we want this to listen on all addresses to
// ensure a reliable connection chance for the client. This is based on lessons
// learned from the sigstore CLI.
func GetRandomPort() (int, error) {
func GetRandomPort() (int32, error) {
listener, err := net.Listen("tcp", ":0") // #nosec
if err != nil {
return 0, err
}
defer listener.Close()

port := listener.Addr().(*net.TCPAddr).Port
return port, nil
// largest TCP port is 2^16, overflow should not happen
// nolint: gosec
return int32(port), nil
}

0 comments on commit 7a0da06

Please sign in to comment.