From b61829b1dfef9b44d8ce9e0019894e8fe54631b1 Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Tue, 8 Oct 2024 09:23:08 +0200 Subject: [PATCH 1/4] errcheck: discard not used error return values --- cmd/icingadb/main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/icingadb/main.go b/cmd/icingadb/main.go index d5fe38f1a..bd94686d7 100644 --- a/cmd/icingadb/main.go +++ b/cmd/icingadb/main.go @@ -50,7 +50,7 @@ func run() int { _ = sdnotify.Ready() logger := logs.GetLogger() - defer logger.Sync() + defer func() { _ = logger.Sync() }() logger.Infof("Starting Icinga DB daemon (%s)", internal.Version.Version) @@ -58,7 +58,7 @@ func run() int { if err != nil { logger.Fatalf("%+v", errors.Wrap(err, "can't create database connection pool from config")) } - defer db.Close() + defer func() { _ = db.Close() }() { logger.Infof("Connecting to database at '%s'", db.GetAddr()) err := db.Ping() @@ -112,7 +112,7 @@ func run() int { if err != nil { logger.Fatalf("%+v", errors.Wrap(err, "can't create database connection pool from config")) } - defer db.Close() + defer func() { _ = db.Close() }() ha = icingadb.NewHA(ctx, db, heartbeat, logs.GetChildLogger("high-availability")) telemetryLogger := logs.GetChildLogger("telemetry") @@ -125,7 +125,7 @@ func run() int { // Give up after 3s, not 5m (default) not to hang for 5m if DB is down. ctx, cancelCtx := context.WithTimeout(context.Background(), 3*time.Second) - ha.Close(ctx) + _ = ha.Close(ctx) cancelCtx() }() s := icingadb.NewSync(db, rc, logs.GetChildLogger("config-sync")) From 18caddbc64de41049ea9ad542d4ecb71d31e9210 Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Tue, 8 Oct 2024 09:25:27 +0200 Subject: [PATCH 2/4] gosec: justify SHA1 usage with #nosec annotations --- cmd/icingadb-migrate/main.go | 2 +- cmd/icingadb-migrate/misc.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/icingadb-migrate/main.go b/cmd/icingadb-migrate/main.go index a0fc5644a..7b04382f0 100644 --- a/cmd/icingadb-migrate/main.go +++ b/cmd/icingadb-migrate/main.go @@ -2,7 +2,7 @@ package main import ( "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- used as a non-cryptographic hash function to hash IDs "database/sql" _ "embed" "encoding/hex" diff --git a/cmd/icingadb-migrate/misc.go b/cmd/icingadb-migrate/misc.go index 3d33ef8c2..8a63825fd 100644 --- a/cmd/icingadb-migrate/misc.go +++ b/cmd/icingadb-migrate/misc.go @@ -2,7 +2,7 @@ package main import ( "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- used as a non-cryptographic hash function to hash IDs "github.com/icinga/icinga-go-library/database" "github.com/icinga/icinga-go-library/objectpacker" "github.com/icinga/icinga-go-library/types" @@ -54,7 +54,7 @@ var objectTypes = map[uint8]string{1: "host", 2: "service"} // hashAny combines objectpacker.PackAny and SHA1 hashing. func hashAny(in interface{}) []byte { - hash := sha1.New() + hash := sha1.New() // #nosec G401 -- used as a non-cryptographic hash function to hash IDs if err := objectpacker.PackAny(in, hash); err != nil { panic(err) } From a54ef678ee9f71a831095f1b98d57ffed1725daa Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Tue, 8 Oct 2024 09:26:50 +0200 Subject: [PATCH 3/4] gosec: handle integer conversions and potential overflows --- cmd/icingadb-migrate/convert.go | 11 ++++++++++- pkg/icingadb/cleanup.go | 11 +++++++---- pkg/icingadb/delta_test.go | 18 +++++++++--------- pkg/icingadb/types/notification_type.go | 10 ++-------- 4 files changed, 28 insertions(+), 22 deletions(-) diff --git a/cmd/icingadb-migrate/convert.go b/cmd/icingadb-migrate/convert.go index 0a875e5c1..faa167139 100644 --- a/cmd/icingadb-migrate/convert.go +++ b/cmd/icingadb-migrate/convert.go @@ -11,6 +11,7 @@ import ( "github.com/icinga/icingadb/pkg/icingadb/v1/history" "github.com/jmoiron/sqlx" "github.com/pkg/errors" + "math" "strconv" "strings" "time" @@ -282,6 +283,14 @@ func convertDowntimeRows( cancelTime = actualEnd } + // The IDO duration is of type bigint, representing seconds, while Icinga DB's FlexibleDuration is an unsigned + // bigint, representing milliseconds. In theory, there should be no negative value in the IDO and multiplied + // with factor 1,000 should not overflow anything. In theory, at least. To make sure, invalid values are capped. + var flexibleDuration uint64 + if durationSec := row.Duration; durationSec >= 0 && durationSec < math.MaxUint64/1_000 { + flexibleDuration = uint64(durationSec) * 1_000 + } + downtimeHistory = append(downtimeHistory, &history.DowntimeHistory{ DowntimeHistoryEntity: history.DowntimeHistoryEntity{DowntimeId: id}, HistoryTableMeta: history.HistoryTableMeta{ @@ -299,7 +308,7 @@ func convertDowntimeRows( Author: row.AuthorName, Comment: row.CommentData, IsFlexible: types.Bool{Bool: row.IsFixed == 0, Valid: true}, - FlexibleDuration: uint64(row.Duration) * 1000, + FlexibleDuration: flexibleDuration, ScheduledStartTime: scheduledStart, ScheduledEndTime: scheduledEnd, StartTime: startTime, diff --git a/pkg/icingadb/cleanup.go b/pkg/icingadb/cleanup.go index 754e5a33d..ca38e3511 100644 --- a/pkg/icingadb/cleanup.go +++ b/pkg/icingadb/cleanup.go @@ -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, @@ -45,7 +45,10 @@ func (stmt *CleanupStmt) CleanupOlderThan( return database.CantPerformQuery(err, q) } - rowsDeleted, err = rs.RowsAffected() + i, err := rs.RowsAffected() + if err == nil && i >= 0 { + rowsDeleted = uint64(i) + } return err }, @@ -57,7 +60,7 @@ 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 { @@ -65,7 +68,7 @@ func (stmt *CleanupStmt) CleanupOlderThan( } } - if rowsDeleted < int64(count) { + if rowsDeleted < count { break } } diff --git a/pkg/icingadb/delta_test.go b/pkg/icingadb/delta_test.go index 5067ecd3a..faacad14b 100644 --- a/pkg/icingadb/delta_test.go +++ b/pkg/icingadb/delta_test.go @@ -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++ { @@ -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 diff --git a/pkg/icingadb/types/notification_type.go b/pkg/icingadb/types/notification_type.go index ce88c2efb..24887810e 100644 --- a/pkg/icingadb/types/notification_type.go +++ b/pkg/icingadb/types/notification_type.go @@ -3,7 +3,6 @@ package types import ( "database/sql/driver" "encoding" - "github.com/icinga/icinga-go-library/types" "github.com/pkg/errors" "strconv" ) @@ -15,17 +14,12 @@ type NotificationType uint16 func (nt *NotificationType) UnmarshalText(text []byte) error { s := string(text) - i, err := strconv.ParseUint(s, 10, 64) + i, err := strconv.ParseUint(s, 10, 16) if err != nil { - return types.CantParseUint64(err, s) + return errors.Wrapf(err, "can't parse %q into uint16", s) } n := NotificationType(i) - if uint64(n) != i { - // Truncated due to above cast, obviously too high - return badNotificationType(s) - } - if _, ok := notificationTypes[n]; !ok { return badNotificationType(s) } From 14d4070ad716ad141801c2ffb3d262e71315a6bf Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Tue, 8 Oct 2024 09:27:31 +0200 Subject: [PATCH 4/4] GHA: introduce golangci-lint The same GitHub Action already used in the icinga-go-library was added to the Go workflow. --- .github/workflows/go.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index b0d2f0e19..b3d61a50a 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -38,6 +38,14 @@ jobs: with: install-go: false + - uses: golangci/golangci-lint-action@v6 + with: + version: latest + only-new-issues: true + + # Enable the gosec linter w/o having to create a .golangci.yml config + args: -E gosec + vet: runs-on: ubuntu-latest steps: