diff --git a/.golangci.yml b/.golangci.yml index 09e3360a538..bd11a3fce53 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -104,6 +104,7 @@ linters: - forcetypeassert # for tests' brevity sake - funlen # tests may be long - gocognit # triggered by subtests + - gosec # not critical, too many things to disable - ireturn # we have exceptions, so need to silence them in tests - lll # tests often require long lines - maintidx # not critical for tests diff --git a/admin/commands/summary.go b/admin/commands/summary.go index 2a900972d90..d5987487c51 100644 --- a/admin/commands/summary.go +++ b/admin/commands/summary.go @@ -189,7 +189,10 @@ func addServerData(ctx context.Context, zipW *zip.Writer, usePprof bool) { addData(zipW, path.Join("server", rf.Name), rf.Modified, rc) //nolint:gosec - rc.Close() //nolint:errcheck + err = rc.Close() + if err != nil { + logrus.Errorf("Failed to close zip archive %s: %v", rf.Name, err) + } } } @@ -277,7 +280,10 @@ func downloadFile(ctx context.Context, zipW *zip.Writer, url, fileName string) e } addData(zipW, path.Join(fileName, rf.Name), rf.Modified, rc) //nolint:gosec - rc.Close() //nolint:errcheck + err = rc.Close() + if err != nil { + logrus.Errorf("Failed to close zip archive %s: %v", rf.Name, err) + } } return nil } diff --git a/agent/agents/mongodb/mongolog/mongodb.go b/agent/agents/mongodb/mongolog/mongodb.go index f87e863c8fe..3a030336dbb 100644 --- a/agent/agents/mongodb/mongolog/mongodb.go +++ b/agent/agents/mongodb/mongolog/mongodb.go @@ -74,7 +74,10 @@ func (m *MongoDB) Run(ctx context.Context) { var log Mongolog defer func() { - log.Stop() //nolint:errcheck + err := log.Stop() + if err != nil { + m.l.Errorf("Can't stop mongolog, reason: %v", err) + } log = nil m.changes <- agents.Change{Status: inventoryv1.AgentStatus_AGENT_STATUS_DONE} close(m.changes) diff --git a/agent/agents/mongodb/profiler/internal/collector/collector.go b/agent/agents/mongodb/profiler/internal/collector/collector.go index 8feba1c0d11..547f455c518 100644 --- a/agent/agents/mongodb/profiler/internal/collector/collector.go +++ b/agent/agents/mongodb/profiler/internal/collector/collector.go @@ -63,7 +63,7 @@ type Collector struct { } // Start starts but doesn't wait until it exits. -func (c *Collector) Start(context.Context) (<-chan proto.SystemProfile, error) { +func (c *Collector) Start(ctx context.Context) (<-chan proto.SystemProfile, error) { c.m.Lock() defer c.m.Unlock() if c.running { @@ -86,7 +86,6 @@ func (c *Collector) Start(context.Context) (<-chan proto.SystemProfile, error) { ready.L.Lock() defer ready.L.Unlock() - ctx := context.Background() labels := pprof.Labels("component", "mongodb.aggregator") go pprof.Do(ctx, labels, func(ctx context.Context) { start( @@ -175,14 +174,14 @@ func connectAndCollect(ctx context.Context, collection *mongo.Collection, dbName logger.Traceln("connect and collect is called") query := createQuery(dbName, startTime) - timeoutCtx, cancel := context.WithTimeout(context.TODO(), cursorTimeout) + timeoutCtx, cancel := context.WithTimeout(ctx, cursorTimeout) defer cancel() cursor, err := createIterator(timeoutCtx, collection, query) if err != nil { logger.Errorf("couldn't create system.profile iterator, reason: %v", err) return } - // do not cancel cursor closing when ctx is canceled + // Ensure cursor is closed even if parent context is canceled to prevent resource leaks. defer cursor.Close(context.Background()) //nolint:errcheck // we got iterator, we are ready @@ -204,7 +203,7 @@ func connectAndCollect(ctx context.Context, collection *mongo.Collection, dbName }() for { - for cursor.TryNext(context.TODO()) { + for cursor.TryNext(ctx) { doc := proto.SystemProfile{} e := cursor.Decode(&doc) if e != nil { diff --git a/agent/agents/mongodb/profiler/internal/collector/collector_test.go b/agent/agents/mongodb/profiler/internal/collector/collector_test.go index dd68f8ae644..458a09dbc87 100644 --- a/agent/agents/mongodb/profiler/internal/collector/collector_test.go +++ b/agent/agents/mongodb/profiler/internal/collector/collector_test.go @@ -178,6 +178,45 @@ func TestCollector(t *testing.T) { assert.Len(t, profiles, maxDocs*maxLoops) } +func TestCollectorContextCancel(t *testing.T) { + url := "mongodb://root:root-password@127.0.0.1:27017" + client, err := createSession(url, "pmm-agent-test") + require.NoError(t, err) + + // Create a sub-context specifically for this test to trigger cancellation. + ctx, cancel := context.WithCancel(t.Context()) + + ctr := New(client, "test_context_cancel", logrus.WithField("component", "collector-test")) + + docsChan, err := ctr.Start(ctx) + require.NoError(t, err) + + // Verify the collector is running. + assert.Equal(t, "collector", ctr.Name()) + + // Cancel the context to signal the internal goroutine to exit. + cancel() + + // Use Stop() as a synchronization point. It waits for the internal WaitGroup, + // which confirms that the start() and connectAndCollect() goroutines have exited. + stopDone := make(chan struct{}) + go func() { + ctr.Stop() + close(stopDone) + }() + + select { + case <-stopDone: + // Success: Internal goroutines shut down gracefully. + case <-time.After(5 * time.Second): + t.Fatal("Collector did not stop via context cancellation within timeout") + } + + // Verify that the data channel was closed. + _, ok := <-docsChan + assert.False(t, ok, "docsChan should be closed after collector stops") +} + func genData(ctx context.Context, client *mongo.Client, maxLoops, maxDocs int) { interval := time.Millisecond diff --git a/agent/agents/mongodb/profiler/internal/parser/parser.go b/agent/agents/mongodb/profiler/internal/parser/parser.go index ff909047fce..12acfa6008b 100644 --- a/agent/agents/mongodb/profiler/internal/parser/parser.go +++ b/agent/agents/mongodb/profiler/internal/parser/parser.go @@ -54,7 +54,7 @@ type Parser struct { } // Start starts but doesn't wait until it exits. -func (p *Parser) Start(context.Context) error { +func (p *Parser) Start(ctx context.Context) error { p.m.Lock() defer p.m.Unlock() if p.running { @@ -70,7 +70,6 @@ func (p *Parser) Start(context.Context) error { p.wg = &sync.WaitGroup{} p.wg.Add(1) - ctx := context.Background() labels := pprof.Labels("component", "mongodb.monitor") go pprof.Do(ctx, labels, func(ctx context.Context) { start( @@ -126,6 +125,8 @@ func start( // PMM-13947 case <-doneChan: return + case <-ctx.Done(): + return default: // just continue if not } @@ -149,6 +150,8 @@ func start( // doneChan needs to be also in separate select statement // as docsChan could be always picked since select picks channels pseudo randomly return + case <-ctx.Done(): + return } } } diff --git a/agent/agents/mongodb/profiler/internal/parser/parser_test.go b/agent/agents/mongodb/profiler/internal/parser/parser_test.go index 21bfbebfc15..dcd48a47f49 100644 --- a/agent/agents/mongodb/profiler/internal/parser/parser_test.go +++ b/agent/agents/mongodb/profiler/internal/parser/parser_test.go @@ -81,6 +81,38 @@ func TestParserStartStop(t *testing.T) { parser1.Stop() } +func TestParserContextCancel(t *testing.T) { + docsChan := make(chan pm.SystemProfile) + a := aggregator.New(time.Now(), "test-id", logrus.WithField("component", "aggregator"), truncate.GetMongoDBDefaultMaxQueryLength()) + + ctx, cancel := context.WithCancel(context.Background()) + parser := New(docsChan, a, logrus.WithField("component", "test-parser")) + + err := parser.Start(ctx) + require.NoError(t, err) + + // Verify parser reported as running + assert.Equal(t, "parser", parser.Name()) + + // Cancel the context to trigger the new shutdown path in start() + cancel() + + // We use Stop() as a synchronization point because it waits for the internal WaitGroup. + // If the context cancellation logic is correct, Stop() should return immediately. + done := make(chan struct{}) + go func() { + parser.Stop() + close(done) + }() + + select { + case <-done: + // Success: the parser stopped correctly via context cancellation + case <-time.After(1 * time.Second): + t.Fatal("Parser did not stop after context cancellation within timeout") + } +} + func TestParserRunning(t *testing.T) { oldInterval := aggregator.DefaultInterval aggregator.DefaultInterval = 10 * time.Second diff --git a/agent/agents/mongodb/profiler/internal/profiler.go b/agent/agents/mongodb/profiler/internal/profiler.go index cc710848829..08218b3a065 100644 --- a/agent/agents/mongodb/profiler/internal/profiler.go +++ b/agent/agents/mongodb/profiler/internal/profiler.go @@ -145,7 +145,10 @@ func (p *Profiler) Stop() error { p.sender.Stop() // close the session; do it after goroutine is closed - p.client.Disconnect(context.TODO()) //nolint:errcheck + err := p.client.Disconnect(context.Background()) + if err != nil { + p.logger.Errorf("Failed to disconnect client from MongoDB, reason: %v", err) + } // set state to "not running" p.running = false diff --git a/agent/agents/mongodb/profiler/mongodb.go b/agent/agents/mongodb/profiler/mongodb.go index 42cd95205f9..ef8cf28701d 100644 --- a/agent/agents/mongodb/profiler/mongodb.go +++ b/agent/agents/mongodb/profiler/mongodb.go @@ -72,7 +72,10 @@ func (m *MongoDB) Run(ctx context.Context) { var prof Profiler defer func() { - prof.Stop() //nolint:errcheck + err := prof.Stop() + if err != nil { + m.l.Errorf("Can't stop profiler, reason: %v", err) + } prof = nil m.changes <- agents.Change{Status: inventoryv1.AgentStatus_AGENT_STATUS_DONE} close(m.changes) diff --git a/agent/agents/mongodb/shared/aggregator/aggregator.go b/agent/agents/mongodb/shared/aggregator/aggregator.go index bc268c5ef98..5896eb669cf 100644 --- a/agent/agents/mongodb/shared/aggregator/aggregator.go +++ b/agent/agents/mongodb/shared/aggregator/aggregator.go @@ -281,7 +281,7 @@ func (a *Aggregator) createResult(_ context.Context) *report.Result { ClientHost: v.Client, AgentId: a.agentID, AgentType: inventoryv1.AgentType_AGENT_TYPE_QAN_MONGODB_PROFILER_AGENT, - PeriodStartUnixSecs: uint32(a.timeStart.Truncate(1 * time.Minute).Unix()), + PeriodStartUnixSecs: uint32(a.timeStart.Truncate(1 * time.Minute).Unix()), //nolint:gosec // Unix timestamp fits in uint32 until 2106 PeriodLengthSecs: uint32(a.d.Seconds()), Example: query, ExampleType: agentv1.ExampleType_EXAMPLE_TYPE_RANDOM, diff --git a/agent/agents/mysql/perfschema/perfschema.go b/agent/agents/mysql/perfschema/perfschema.go index a80c38fc206..834120e5bab 100644 --- a/agent/agents/mysql/perfschema/perfschema.go +++ b/agent/agents/mysql/perfschema/perfschema.go @@ -193,7 +193,10 @@ func newPerfSchema(params *newPerfSchemaParams) (*PerfSchema, error) { // Run extracts performance data and sends it to the channel until ctx is canceled. func (m *PerfSchema) Run(ctx context.Context) { defer func() { - m.dbCloser.Close() //nolint:errcheck + err := m.dbCloser.Close() + if err != nil { + m.l.WithError(err).Error("Failed to close database connection") + } m.changes <- agents.Change{Status: inventoryv1.AgentStatus_AGENT_STATUS_DONE} close(m.changes) }() @@ -345,7 +348,7 @@ func (m *PerfSchema) getNewBuckets(periodStart time.Time, periodLengthSecs uint3 } buckets := makeBuckets(current, prev, m.l, m.maxQueryLength) - startS := uint32(periodStart.Unix()) + startS := uint32(periodStart.Unix()) //nolint:gosec // Unix timestamp fits in uint32 until 2106 m.l.Debugf("Made %d buckets out of %d summaries in %s+%d interval.", len(buckets), len(current), periodStart.Format("15:04:05"), periodLengthSecs) diff --git a/agent/agents/mysql/slowlog/slowlog.go b/agent/agents/mysql/slowlog/slowlog.go index b1ddf9f5e8f..ebe889eb99a 100644 --- a/agent/agents/mysql/slowlog/slowlog.go +++ b/agent/agents/mysql/slowlog/slowlog.go @@ -454,7 +454,7 @@ func makeBuckets( ClientHost: v.Host, AgentId: agentID, AgentType: inventoryv1.AgentType_AGENT_TYPE_QAN_MYSQL_SLOWLOG_AGENT, - PeriodStartUnixSecs: uint32(periodStart.Unix()), + PeriodStartUnixSecs: uint32(periodStart.Unix()), //nolint:gosec // Unix timestamp fits in uint32 until 2106 PeriodLengthSecs: periodLengthSecs, NumQueries: float32(v.TotalQueries), Errors: errListsToMap(v.ErrorsCode, v.ErrorsCount), diff --git a/agent/agents/postgres/pgstatmonitor/pgstatmonitor.go b/agent/agents/postgres/pgstatmonitor/pgstatmonitor.go index 4b16bae0f8f..3aed4d2889b 100644 --- a/agent/agents/postgres/pgstatmonitor/pgstatmonitor.go +++ b/agent/agents/postgres/pgstatmonitor/pgstatmonitor.go @@ -287,7 +287,10 @@ func getPGMonitorVersion(q *reform.Querier) (pgStatMonitorVersion, pgStatMonitor // Run extracts stats data and sends it to the channel until ctx is canceled. func (m *PGStatMonitorQAN) Run(ctx context.Context) { defer func() { - m.dbCloser.Close() //nolint:errcheck + err := m.dbCloser.Close() + if err != nil { + m.l.WithError(err).Error("Failed to close DB connection") + } m.changes <- agents.Change{Status: inventoryv1.AgentStatus_AGENT_STATUS_DONE} close(m.changes) }() @@ -609,7 +612,7 @@ func (m *PGStatMonitorQAN) makeBuckets(current, cache map[time.Time]map[string]* NumQueries: count, ClientHost: currentPSM.ClientIP, AgentType: inventoryv1.AgentType_AGENT_TYPE_QAN_POSTGRESQL_PGSTATMONITOR_AGENT, - PeriodStartUnixSecs: uint32(currentPSM.BucketStartTime.Unix()), + PeriodStartUnixSecs: uint32(currentPSM.BucketStartTime.Unix()), //nolint:gosec // Unix timestamp fits in uint32 until 2106 }, Postgresql: &agentv1.MetricsBucket_PostgreSQL{}, } @@ -725,7 +728,11 @@ func (m *PGStatMonitorQAN) makeBuckets(current, cache map[time.Time]map[string]* func parseHistogramFromRespCalls(respCalls pq.StringArray, prevRespCalls pq.StringArray, vPGSM pgStatMonitorVersion) ([]*agentv1.HistogramItem, error) { histogram := getHistogramRangesArray(vPGSM) for k, v := range respCalls { - val, err := strconv.ParseInt(v, 10, 32) + if k >= len(histogram) { + break + } + // Use ParseUint with bitSize 32 to ensure non-negative values that fit in uint32 + val, err := strconv.ParseUint(v, 10, 32) if err != nil { return nil, fmt.Errorf("failed to parse histogram: %w", err) } @@ -734,12 +741,21 @@ func parseHistogramFromRespCalls(respCalls pq.StringArray, prevRespCalls pq.Stri } for k, v := range prevRespCalls { - val, err := strconv.ParseInt(v, 10, 32) + if k >= len(histogram) { + break + } + val, err := strconv.ParseUint(v, 10, 32) if err != nil { return nil, fmt.Errorf("failed to parse histogram: %w", err) } - histogram[k].Frequency -= uint32(val) + uVal := uint32(val) + if histogram[k].Frequency >= uVal { + histogram[k].Frequency -= uVal + } else { + // Handle counter resets or inconsistencies by capping at 0 + histogram[k].Frequency = 0 + } } return histogram, nil diff --git a/agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go b/agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go index 6ba332b162e..972661d8e33 100644 --- a/agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go +++ b/agent/agents/postgres/pgstatmonitor/pgstatmonitor_test.go @@ -25,7 +25,7 @@ import ( "time" ver "github.com/hashicorp/go-version" - _ "github.com/lib/pq" + "github.com/lib/pq" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -594,3 +594,83 @@ func TestPGStatMonitorSchema(t *testing.T) { assert.Regexp(t, `\d{1,3}.\d{1,3}.\d{1,3}.\d{1,3}`, actual.Common.ClientHost) }) } + +func TestParseHistogramFromRespCalls(t *testing.T) { + t.Parallel() + + vPGSM := pgStatMonitorVersion20PG12 // This version expects 22 histogram buckets + + t.Run("Normal", func(t *testing.T) { + t.Parallel() + current := pq.StringArray{"10", "20", "30"} + prev := pq.StringArray{"5", "10", "15"} + res, err := parseHistogramFromRespCalls(current, prev, vPGSM) + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, uint32(5), res[0].Frequency) + assert.Equal(t, uint32(10), res[1].Frequency) + assert.Equal(t, uint32(15), res[2].Frequency) + }) + + t.Run("MoreBucketsThanExpected", func(t *testing.T) { + t.Parallel() + // Create more items than the internal getHistogramRangesArray provides + largeResp := make(pq.StringArray, 50) + for i := range largeResp { + largeResp[i] = "1" + } + res, err := parseHistogramFromRespCalls(largeResp, nil, vPGSM) + require.NoError(t, err) + // Should not panic and should cap at the length of our static ranges + expectedLen := len(getHistogramRangesArray(vPGSM)) + assert.Len(t, res, expectedLen) + }) + + t.Run("CounterReset", func(t *testing.T) { + t.Parallel() + // Previous values higher than current (e.g. pg_stat_monitor_reset called) + current := pq.StringArray{"10"} + prev := pq.StringArray{"20"} + res, err := parseHistogramFromRespCalls(current, prev, vPGSM) + require.NoError(t, err) + // Should be 0, not a huge wrapped-around uint32 + assert.Equal(t, uint32(0), res[0].Frequency) + }) + + t.Run("InvalidData", func(t *testing.T) { + t.Parallel() + current := pq.StringArray{"not-a-number"} + res, err := parseHistogramFromRespCalls(current, nil, vPGSM) + require.Error(t, err) + assert.Nil(t, res) + }) + + t.Run("NegativeValues", func(t *testing.T) { + t.Parallel() + // ParseUint should fail on negative numbers + current := pq.StringArray{"-1"} + res, err := parseHistogramFromRespCalls(current, nil, vPGSM) + require.Error(t, err) + assert.Nil(t, res) + }) +} + +func TestGetHistogramRangesArray(t *testing.T) { + t.Parallel() + + t.Run("Version20Plus", func(t *testing.T) { + t.Parallel() + res := getHistogramRangesArray(pgStatMonitorVersion20PG12) + assert.Len(t, res, 22) + assert.Equal(t, "(0 - 1)", res[0].Range) + assert.Equal(t, "(100000 - ...)", res[21].Range) + }) + + t.Run("OldVersion", func(t *testing.T) { + t.Parallel() + res := getHistogramRangesArray(pgStatMonitorVersion09) + assert.Len(t, res, 10) + assert.Equal(t, "(0 - 3)", res[0].Range) + assert.Equal(t, "(31622 - 100000)", res[9].Range) + }) +} diff --git a/agent/agents/postgres/pgstatstatements/pgstatstatements.go b/agent/agents/postgres/pgstatstatements/pgstatstatements.go index 51a93bf1959..026707e074d 100644 --- a/agent/agents/postgres/pgstatstatements/pgstatstatements.go +++ b/agent/agents/postgres/pgstatstatements/pgstatstatements.go @@ -155,7 +155,10 @@ func getPgStatVersion(q *reform.Querier) (semver.Version, error) { // Run extracts stats data and sends it to the channel until ctx is canceled. func (m *PGStatStatementsQAN) Run(ctx context.Context) { defer func() { - m.dbCloser.Close() //nolint:errcheck + err := m.dbCloser.Close() + if err != nil { + m.l.WithError(err).Error("Failed to close DB connection") + } m.changes <- agents.Change{Status: inventoryv1.AgentStatus_AGENT_STATUS_DONE} close(m.changes) }() @@ -312,7 +315,13 @@ func (m *PGStatStatementsQAN) getNewBuckets(ctx context.Context, periodStart tim } buckets := m.makeBuckets(current, prev) - startS := uint32(periodStart.Unix()) + unix := periodStart.Unix() + if unix < 0 { + unix = 0 + } else if unix > math.MaxUint32 { + unix = math.MaxUint32 + } + startS := uint32(unix) m.l.Debugf("Made %d buckets out of %d stat statements in %s+%d interval.", len(buckets), len(current), periodStart.Format("15:04:05"), periodLengthSecs) diff --git a/agent/cmd/pmm-agent-entrypoint/main.go b/agent/cmd/pmm-agent-entrypoint/main.go index e83c099bcd9..26d2b1f3ce2 100644 --- a/agent/cmd/pmm-agent-entrypoint/main.go +++ b/agent/cmd/pmm-agent-entrypoint/main.go @@ -113,7 +113,9 @@ func runPmmAgent(ctx context.Context, commandLineArgs []string, restartPolicy re func commandPmmAgent(args []string) *exec.Cmd { const pmmAgentCommandName = "pmm-agent" - command := exec.Command(pmmAgentCommandName, args...) + // Gosec G204 warns about launching subprocesses with variable arguments. + // Here the binary name is constant and args are passed directly (no shell). + command := exec.Command(pmmAgentCommandName, args...) //nolint:gosec,noctx command.Stdout = os.Stdout command.Stderr = os.Stderr return command diff --git a/agent/runner/actions/postgresql_show_create_table_action.go b/agent/runner/actions/postgresql_show_create_table_action.go index 16359755ea4..e659059b11d 100644 --- a/agent/runner/actions/postgresql_show_create_table_action.go +++ b/agent/runner/actions/postgresql_show_create_table_action.go @@ -66,6 +66,7 @@ type indexInfo struct { } type postgresqlShowCreateTableAction struct { + l *logrus.Entry id string timeout time.Duration params *agentv1.StartActionRequest_PostgreSQLShowCreateTableParams @@ -88,6 +89,7 @@ func NewPostgreSQLShowCreateTableAction( } return &postgresqlShowCreateTableAction{ + l: logrus.WithField("component", postgreSQLShowCreateTableActionType), id: id, timeout: timeout, params: params, @@ -118,14 +120,19 @@ func (a *postgresqlShowCreateTableAction) DSN() string { // Run runs an Action and returns output and error. func (a *postgresqlShowCreateTableAction) Run(ctx context.Context) ([]byte, error) { - defer templates.CleanupTempDir(a.tmpDir, logrus.WithField("component", postgreSQLShowCreateTableActionType)) + defer templates.CleanupTempDir(a.tmpDir, a.l) connector, err := pq.NewConnector(a.dsn) if err != nil { return nil, err } db := sql.OpenDB(connector) - defer db.Close() //nolint:errcheck + defer func(db *sql.DB) { + closeErr := db.Close() + if closeErr != nil { + a.l.Errorf("Failed to close db: %v", closeErr) + } + }(db) var buf bytes.Buffer // Extract table id @@ -191,7 +198,10 @@ func (a *postgresqlShowCreateTableAction) printTableInit(ctx context.Context, w } return "", err } - fmt.Fprintf(w, "Table \"%s.%s\"\n", schema, relname) + _, err = fmt.Fprintf(w, "Table \"%s.%s\"\n", schema, relname) + if err != nil { + a.l.Errorf("Failed to write table info into io.Writer: %v", err) + } return tableID, nil } @@ -224,11 +234,19 @@ ORDER BY a.attnum;`, tableID) if err != nil { return err } - defer rows.Close() //nolint:errcheck + defer func(rows *sql.Rows) { + closeErr := rows.Close() + if closeErr != nil { + a.l.Errorf("Failed to close rows: %v", closeErr) + } + }(rows) tw := tabwriter.NewWriter(w, 0, 0, 1, ' ', tabwriter.Debug) - fmt.Fprintln(tw, "Column\tType\tCollation\tNullable\tDefault\tStorage\tStats target\tDescription") //nolint:errcheck + _, err = fmt.Fprintln(tw, "Column\tType\tCollation\tNullable\tDefault\tStorage\tStats target\tDescription") + if err != nil { + a.l.Errorf("Failed to write header into tabwriter: %v", err) + } for rows.Next() { var ci columnInfo @@ -247,7 +265,7 @@ ORDER BY a.attnum;`, tableID) return err } - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + _, err = fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", ci.Attname, ci.FormatType, pointer.GetString(ci.Attcollation), @@ -256,6 +274,9 @@ ORDER BY a.attnum;`, tableID) formatStorage(ci.Attstorage), pointer.GetString(ci.Attstattarget), pointer.GetString(ci.ColDescription)) + if err != nil { + a.l.Errorf("Failed to write column info into tabwriter: %v", err) + } } err = rows.Err() if err != nil { @@ -290,13 +311,21 @@ ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname`, tableID) if err != nil { return err } - defer rows.Close() //nolint:errcheck + defer func(rows *sql.Rows) { + closeErr := rows.Close() + if closeErr != nil { + a.l.Errorf("Failed to close rows: %v", closeErr) + } + }(rows) var buf bytes.Buffer // We need it to be able to call Flush method to not write header if there are no rows. bw := bufio.NewWriter(&buf) - fmt.Fprintln(bw, "Indexes:") //nolint:errcheck + _, err = fmt.Fprintln(bw, "Indexes:") + if err != nil { + a.l.Errorf("Failed to write header into buffer: %v", err) + } for rows.Next() { info := indexInfo{} @@ -318,20 +347,35 @@ ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname`, tableID) if err != nil { return err } - fmt.Fprintf(bw, "\t%q", info.Relname) + _, err = fmt.Fprintf(bw, "\t%q", info.Relname) + if err != nil { + a.l.Errorf("Failed to write index name into buffer: %v", err) + } //nolint:nestif if pointer.GetString(info.Contype) == "x" { - fmt.Fprintf(bw, " %s", pointer.GetString(info.PgGetConstraintDef)) + _, err = fmt.Fprintf(bw, " %s", pointer.GetString(info.PgGetConstraintDef)) + if err != nil { + a.l.Errorf("Failed to write 'EXCLUDE' info into buffer: %v", err) + } } else { // Label as primary key or unique (but not both). if info.IsPrimary { - fmt.Fprintf(bw, " PRIMARY KEY,") + _, err = fmt.Fprintf(bw, " PRIMARY KEY,") + if err != nil { + a.l.Errorf("Failed to write 'PRIMARY KEY' info into buffer: %v", err) + } } else if info.IsUnique { if pointer.GetString(info.Contype) == "u" { - fmt.Fprintf(bw, " UNIQUE CONSTRAINT,") + _, err = fmt.Fprintf(bw, " UNIQUE CONSTRAINT,") + if err != nil { + a.l.Errorf("Failed to write 'UNIQUE CONSTRAINT' info into buffer: %v", err) + } } else { - fmt.Fprintf(bw, " UNIQUE,") + _, err = fmt.Fprintf(bw, " UNIQUE,") + if err != nil { + a.l.Errorf("Failed to write 'UNIQUE' info into buffer: %v", err) + } } } @@ -341,18 +385,30 @@ ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname`, tableID) if usingPos != -1 { indexDef = indexDef[usingPos+7:] } - fmt.Fprintf(bw, " %s", indexDef) + _, err = fmt.Fprintf(bw, " %s", indexDef) + if err != nil { + a.l.Errorf("Failed to write indexDef info into buffer: %v", err) + } // Need these for deferrable PK/UNIQUE indexes. if pointer.GetBool(info.Condeferrable) { - fmt.Fprintf(bw, " DEFERRABLE") + _, err = fmt.Fprintf(bw, " DEFERRABLE") + if err != nil { + a.l.Errorf("Failed to write 'DEFERRABLE' info into buffer: %v", err) + } } if pointer.GetBool(info.Condeferred) { - fmt.Fprintf(bw, " INITIALLY DEFERRED") + _, err = fmt.Fprintf(bw, " INITIALLY DEFERRED") + if err != nil { + a.l.Errorf("Failed to write 'INITIALLY DEFERRED' info into buffer: %v", err) + } } } - fmt.Fprintf(bw, "\n") + _, err = fmt.Fprintf(bw, "\n") + if err != nil { + a.l.Errorf("Failed to write '\n' info into buffer: %v", err) + } err = bw.Flush() if err != nil { return err @@ -362,7 +418,10 @@ ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname`, tableID) if err != nil { return err } - w.Write(buf.Bytes()) //nolint:errcheck + _, err = w.Write(buf.Bytes()) + if err != nil { + a.l.Errorf("Failed to write buffer into io.Writer: %v", err) + } return nil } @@ -376,13 +435,21 @@ ORDER BY conname`, tableID) if err != nil { return err } - defer rows.Close() //nolint:errcheck + defer func(rows *sql.Rows) { + closeErr := rows.Close() + if closeErr != nil { + a.l.Errorf("Failed to close rows: %v", closeErr) + } + }(rows) var buf bytes.Buffer // We need it to be able to call Flush method to not write header if there are no rows. bw := bufio.NewWriter(&buf) - fmt.Fprintln(bw, "Foreign-key constraints:") //nolint:errcheck + _, err = fmt.Fprintln(bw, "Foreign-key constraints:") + if err != nil { + a.l.Errorf("Failed to write header into buffer: %v", err) + } for rows.Next() { var conname, condef string @@ -393,7 +460,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - fmt.Fprintf(bw, "\t%q %s\n", conname, condef) + _, err = fmt.Fprintf(bw, "\t%q %s\n", conname, condef) + if err != nil { + a.l.Errorf("Failed to write conname into buffer: %v", err) + } err = bw.Flush() if err != nil { @@ -404,7 +474,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - w.Write(buf.Bytes()) //nolint:errcheck + _, err = w.Write(buf.Bytes()) + if err != nil { + a.l.Errorf("Failed to write buffer into io.Writer: %v", err) + } return nil } @@ -419,13 +492,21 @@ ORDER BY conname`, tableID) if err != nil { return err } - defer rows.Close() //nolint:errcheck + defer func(rows *sql.Rows) { + closeErr := rows.Close() + if closeErr != nil { + a.l.Errorf("Failed to close rows: %v", closeErr) + } + }(rows) var buf bytes.Buffer // We need it to be able to call Flush method to not write header if there are no rows. bw := bufio.NewWriter(&buf) - fmt.Fprintln(bw, "Referenced by:") //nolint:errcheck + _, err = fmt.Fprintln(bw, "Referenced by:") + if err != nil { + a.l.Errorf("Failed to write header into buffer: %v", err) + } for rows.Next() { var conname, conrelid, condef string @@ -437,7 +518,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - fmt.Fprintf(bw, "\tTABLE %q CONSTRAINT %q %s\n", conrelid, conname, condef) + _, err = fmt.Fprintf(bw, "\tTABLE %q CONSTRAINT %q %s\n", conrelid, conname, condef) + if err != nil { + a.l.Errorf("Failed to write table constraint into buffer: %v", err) + } err = bw.Flush() if err != nil { @@ -448,7 +532,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - w.Write(buf.Bytes()) //nolint:errcheck + _, err = w.Write(buf.Bytes()) + if err != nil { + a.l.Errorf("Failed to write buffer into io.Writer: %v", err) + } return nil } @@ -462,13 +549,21 @@ ORDER BY conname`, tableID) if err != nil { return err } - defer rows.Close() //nolint:errcheck + defer func(rows *sql.Rows) { + closeErr := rows.Close() + if closeErr != nil { + a.l.Errorf("Failed to close rows: %v", closeErr) + } + }(rows) var buf bytes.Buffer // We need it to be able to call Flush method to not write header if there are no rows. bw := bufio.NewWriter(&buf) - fmt.Fprintln(bw, "Check constraints:") //nolint:errcheck + _, err = fmt.Fprintln(bw, "Check constraints:") + if err != nil { + a.l.Errorf("Failed to write header into buffer: %v", err) + } for rows.Next() { var conname, condef string @@ -479,7 +574,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - fmt.Fprintf(bw, "\t%q %s\n", conname, condef) + _, err = fmt.Fprintf(bw, "\t%q %s\n", conname, condef) + if err != nil { + a.l.Errorf("Failed to write conname into buffer: %v", err) + } err = bw.Flush() if err != nil { @@ -490,7 +588,10 @@ ORDER BY conname`, tableID) if err != nil { return err } - w.Write(buf.Bytes()) //nolint:errcheck + _, err = w.Write(buf.Bytes()) + if err != nil { + a.l.Errorf("Failed to write buffer into io.Writer: %v", err) + } return nil } diff --git a/agent/runner/actions/pt_mysql_summary_action.go b/agent/runner/actions/pt_mysql_summary_action.go index 33cce7fa48c..05cfdf3a24d 100644 --- a/agent/runner/actions/pt_mysql_summary_action.go +++ b/agent/runner/actions/pt_mysql_summary_action.go @@ -97,7 +97,10 @@ func (a *ptMySQLSummaryAction) Run(ctx context.Context) ([]byte, error) { if err != nil { return nil, fmt.Errorf("failed to write to temporary file: %w", err) } - tmpFile.Close() //nolint:errcheck + err = tmpFile.Close() + if err != nil { + return nil, fmt.Errorf("failed to close temporary file: %w", err) + } cmd := exec.CommandContext(ctx, a.command, "--defaults-file="+tmpFile.Name()) //nolint:gosec cmd.Env = []string{"PATH=" + os.Getenv("PATH")} diff --git a/agent/runner/jobs/mongodb_backup_job.go b/agent/runner/jobs/mongodb_backup_job.go index af2c383dbdd..418de775f84 100644 --- a/agent/runner/jobs/mongodb_backup_job.go +++ b/agent/runner/jobs/mongodb_backup_job.go @@ -123,7 +123,12 @@ func (j *MongoDBBackupJob) Run(ctx context.Context, send Send) error { if err != nil { return err } - defer os.Remove(confFile) //nolint:errcheck + defer func(name string) { + remErr := os.Remove(name) + if remErr != nil { + j.l.Errorf("Failed to remove pbm config file %s: %v", name, remErr) + } + }(confFile) configParams := pbmConfigParams{ configFilePath: confFile, diff --git a/agent/runner/jobs/mongodb_restore_job.go b/agent/runner/jobs/mongodb_restore_job.go index e56aacc2b92..a99a7ee4fc0 100644 --- a/agent/runner/jobs/mongodb_restore_job.go +++ b/agent/runner/jobs/mongodb_restore_job.go @@ -123,7 +123,12 @@ func (j *MongoDBRestoreJob) Run(ctx context.Context, send Send) error { if err != nil { return fmt.Errorf("failed to write to PBM config: %w", err) } - defer os.Remove(confFile) //nolint:errcheck + defer func(name string) { + remErr := os.Remove(name) + if remErr != nil { + j.l.Errorf("Failed to remove pbm config file %s: %v", name, remErr) + } + }(confFile) configParams := pbmConfigParams{ configFilePath: confFile, diff --git a/agent/runner/jobs/mysql_restore_job.go b/agent/runner/jobs/mysql_restore_job.go index df2b41fdc4b..4437e431292 100644 --- a/agent/runner/jobs/mysql_restore_job.go +++ b/agent/runner/jobs/mysql_restore_job.go @@ -292,7 +292,11 @@ func mySQLActive(ctx context.Context, mySQLServiceName string) (bool, error) { ctx, cancel := context.WithTimeout(ctx, systemctlTimeout) defer cancel() - cmd := exec.CommandContext(ctx, "systemctl", "is-active", "--quiet", mySQLServiceName) + // In this file, the mySQLServiceName is retrieved via the getMysqlServiceName function. + // This function executes systemctl list-unit-files and filters the output using a strictly + // defined regular expression: mysql(d)?\.service. Because the value is strictly validated + // to be either mysql.service or mysqld.service, this is a false positive. + cmd := exec.CommandContext(ctx, "systemctl", "is-active", "--quiet", mySQLServiceName) //nolint:gosec err := cmd.Start() if err != nil { return false, fmt.Errorf("starting systemctl is-active command failed: %w", err) @@ -315,7 +319,11 @@ func stopMySQL(ctx context.Context, mySQLServiceName string) error { ctx, cancel := context.WithTimeout(ctx, systemctlTimeout) defer cancel() - cmd := exec.CommandContext(ctx, "systemctl", "stop", mySQLServiceName) + // In this file, the mySQLServiceName is retrieved via the getMysqlServiceName function. + // This function executes systemctl list-unit-files and filters the output using a strictly + // defined regular expression: mysql(d)?\.service. Because the value is strictly validated + // to be either mysql.service or mysqld.service, this is a false positive. + cmd := exec.CommandContext(ctx, "systemctl", "stop", mySQLServiceName) //nolint:gosec err := cmd.Start() if err != nil { return fmt.Errorf("starting systemctl stop command failed: %w", err) @@ -332,7 +340,11 @@ func startMySQL(ctx context.Context, mySQLServiceName string) error { ctx, cancel := context.WithTimeout(ctx, systemctlTimeout) defer cancel() - cmd := exec.CommandContext(ctx, "systemctl", "start", mySQLServiceName) + // In this file, the mySQLServiceName is retrieved via the getMysqlServiceName function. + // This function executes systemctl list-unit-files and filters the output using a strictly + // defined regular expression: mysql(d)?\.service. Because the value is strictly validated + // to be either mysql.service or mysqld.service, this is a false positive. + cmd := exec.CommandContext(ctx, "systemctl", "start", mySQLServiceName) //nolint:gosec err := cmd.Start() if err != nil { return fmt.Errorf("starting systemctl start command failed: %w", err) diff --git a/agent/runner/jobs/pbm_helpers.go b/agent/runner/jobs/pbm_helpers.go index 0f77b4e046b..62ffe588577 100644 --- a/agent/runner/jobs/pbm_helpers.go +++ b/agent/runner/jobs/pbm_helpers.go @@ -203,7 +203,11 @@ func execPBMCommand(ctx context.Context, dsn string, to any, args ...string) err defer cancel() args = append(args, "--out=json", "--mongodb-uri="+dsn) - cmd := exec.CommandContext(nCtx, pbmBin, args...) + // In this case the arguments are constructed internally within the agent's job runner. + // Since the execution is controlled and the binary name is a known internal variable, + // this is considered a false positive. Other instances of command execution + // in this file already use the //nolint:gosec directive for the same reason. + cmd := exec.CommandContext(nCtx, pbmBin, args...) //nolint:gosec b, err := cmd.Output() if err != nil { @@ -701,17 +705,33 @@ func writePBMConfigFile(conf *PBMConfig) (string, error) { if err != nil { return "", fmt.Errorf("failed to create pbm configuration file: %w", err) } + removeConfFunc := func() { + remErr := os.Remove(tmp.Name()) + if remErr != nil { + logrus.Errorf("Failed to remove pbm config file %s: %v", tmp.Name(), remErr) + } + } bytes, err := yaml.Marshal(&conf) if err != nil { - tmp.Close() //nolint:errcheck - return "", fmt.Errorf("failed to marshal pbm configuration: %w", err) + defer removeConfFunc() + err = fmt.Errorf("failed to marshal pbm configuration: %w", err) + closeErr := tmp.Close() + if closeErr != nil { + return "", errors.Join(err, fmt.Errorf("failed to close pbm configuration file: %w", closeErr)) + } + return "", err } _, err = tmp.Write(bytes) if err != nil { - tmp.Close() //nolint:errcheck - return "", fmt.Errorf("failed to write pbm configuration file: %w", err) + defer removeConfFunc() + err = fmt.Errorf("failed to write pbm configuration file: %w", err) + closeErr := tmp.Close() + if closeErr != nil { + return "", errors.Join(err, fmt.Errorf("failed to close pbm configuration file: %w", closeErr)) + } + return "", err } return tmp.Name(), tmp.Close() diff --git a/agent/versioner/versioner.go b/agent/versioner/versioner.go index 9951b99532c..e7a60ac550f 100644 --- a/agent/versioner/versioner.go +++ b/agent/versioner/versioner.go @@ -67,7 +67,12 @@ func (RealExecFunctions) LookPath(file string) (string, error) { // CommandContext calls Go's implementation of the CommandContext() function. func (RealExecFunctions) CommandContext(ctx context.Context, name string, arg ...string) CombinedOutputer { //nolint:ireturn - return exec.CommandContext(ctx, name, arg...) + // In the context of this file, the RealExecFunctions struct acts as a thin wrapper + // around the standard os/exec library to facilitate testing via dependency injection. + // Since this is a generic wrapper and the actual values passed to it + // (like mysqld, xtrabackup, etc.) are defined as internal constants within the package, + // this is considered a false positive. The risk is managed by the caller, not the wrapper itself. + return exec.CommandContext(ctx, name, arg...) //nolint:gosec } // Versioner implements version retrieving functions for different software.