Skip to content

Commit

Permalink
gosec: handle integer conversions and potential overflows
Browse files Browse the repository at this point in the history
  • Loading branch information
oxzi committed Oct 8, 2024
1 parent 28b06b7 commit 2184aff
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 19 deletions.
2 changes: 1 addition & 1 deletion cmd/icingadb-migrate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ func convertDowntimeRows(
Author: row.AuthorName,
Comment: row.CommentData,
IsFlexible: types.Bool{Bool: row.IsFixed == 0, Valid: true},
FlexibleDuration: uint64(row.Duration) * 1000,
FlexibleDuration: uint64(row.Duration) * 1000, // #nosec G115 -- flexible_duration is unsigned in Icinga DB's schema
ScheduledStartTime: scheduledStart,
ScheduledEndTime: scheduledEnd,
StartTime: startTime,
Expand Down
9 changes: 5 additions & 4 deletions pkg/icingadb/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (stmt *CleanupStmt) CleanupOlderThan(
defer db.Log(ctx, q, &counter).Stop()

for {
var rowsDeleted int64
var rowsDeleted uint64

err := retry.WithBackoff(
ctx,
Expand All @@ -45,7 +45,8 @@ func (stmt *CleanupStmt) CleanupOlderThan(
return database.CantPerformQuery(err, q)
}

rowsDeleted, err = rs.RowsAffected()
i, err := rs.RowsAffected()
rowsDeleted = uint64(i) // #nosec G115 -- negative amount of rows should not be possible

return err
},
Expand All @@ -57,15 +58,15 @@ func (stmt *CleanupStmt) CleanupOlderThan(
return 0, err
}

counter.Add(uint64(rowsDeleted))
counter.Add(rowsDeleted)

for _, onSuccess := range onSuccess {
if err := onSuccess(ctx, make([]struct{}, rowsDeleted)); err != nil {
return 0, err
}
}

if rowsDeleted < int64(count) {
if rowsDeleted < count {
break
}
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/icingadb/delta_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,14 +208,14 @@ func testDeltaVerifyResult(t *testing.T, name string, expected map[uint64]uint64
}

func BenchmarkDelta(b *testing.B) {
for n := 1 << 10; n <= 1<<20; n <<= 1 {
b.Run(strconv.Itoa(n), func(b *testing.B) {
for n := uint64(1 << 10); n <= 1<<20; n <<= 1 {
b.Run(strconv.FormatUint(n, 10), func(b *testing.B) {
benchmarkDelta(b, n)
})
}
}

func benchmarkDelta(b *testing.B, numEntities int) {
func benchmarkDelta(b *testing.B, numEntities uint64) {
chActual := make([]chan database.Entity, b.N)
chDesired := make([]chan database.Entity, b.N)
for i := 0; i < b.N; i++ {
Expand All @@ -231,20 +231,20 @@ func benchmarkDelta(b *testing.B, numEntities int) {
binary.BigEndian.PutUint64(e.PropertiesChecksum, checksum)
return e
}
for i := 0; i < numEntities; i++ {
for i := uint64(0); i < numEntities; i++ {
// each iteration writes exactly one entity to each channel
var eActual, eDesired database.Entity
switch i % 3 {
case 0: // distinct IDs
eActual = makeEndpoint(1, uint64(i), uint64(i))
eDesired = makeEndpoint(2, uint64(i), uint64(i))
eActual = makeEndpoint(1, i, i)
eDesired = makeEndpoint(2, i, i)
case 1: // same ID, same checksum
e := makeEndpoint(3, uint64(i), uint64(i))
e := makeEndpoint(3, i, i)
eActual = e
eDesired = e
case 2: // same ID, different checksum
eActual = makeEndpoint(4, uint64(i), uint64(i))
eDesired = makeEndpoint(4, uint64(i), uint64(i+1))
eActual = makeEndpoint(4, i, i)
eDesired = makeEndpoint(4, i, i+1)
}
for _, ch := range chActual {
ch <- eActual
Expand Down
8 changes: 7 additions & 1 deletion pkg/icingadb/history/retention.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,12 @@ func (r *Retention) Start(ctx context.Context) error {
r.logger.Debugf("Skipping history retention for category %s", stmt.Category)
continue
}
if days > 1<<16 {
r.logger.Warnf(
"Skipping history retention for category %s as %d days may overflow, leading to undesired retention behavior",
stmt.Category, days)
continue
}

r.logger.Debugw(
fmt.Sprintf("Starting history retention for category %s", stmt.Category),
Expand All @@ -182,7 +188,7 @@ func (r *Retention) Start(ctx context.Context) error {

stmt := stmt
periodic.Start(ctx, r.interval, func(tick periodic.Tick) {
olderThan := tick.Time.AddDate(0, 0, -int(days))
olderThan := tick.Time.AddDate(0, 0, -int(days)) // #nosec G115 -- if this overflows, AddDate will overflow as well

r.logger.Debugf("Cleaning up historical data for category %s from table %s older than %s",
stmt.Category, stmt.Table, olderThan)
Expand Down
7 changes: 3 additions & 4 deletions pkg/icingadb/types/notification_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding"
"github.com/icinga/icinga-go-library/types"
"github.com/pkg/errors"
"math"
"strconv"
)

Expand All @@ -19,13 +20,11 @@ func (nt *NotificationType) UnmarshalText(text []byte) error {
if err != nil {
return types.CantParseUint64(err, s)
}

n := NotificationType(i)
if uint64(n) != i {
// Truncated due to above cast, obviously too high
if i > math.MaxUint16 {
return badNotificationType(s)
}

n := NotificationType(i)
if _, ok := notificationTypes[n]; !ok {
return badNotificationType(s)
}
Expand Down

0 comments on commit 2184aff

Please sign in to comment.