From eeb15df70be5f2220f543366edaced91988e2029 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Fri, 14 Jan 2022 18:22:04 -0500 Subject: [PATCH] release-21.2: backupccl,jobs: handle panics from invalid cron parsing This change changes all calls to `func (expr *Expression) Next` in the `gorhill/cronexpr` package to use a utility method instead, that handles panics that might be thrown by the underlying library. Fixes: #74873 Release justification (bug fix): certain invalid cron expr could result in a panic from the undelying cronexpr parsing libarary causing the node to crash --- pkg/ccl/backupccl/create_scheduled_backup.go | 11 +++++-- .../backupccl/create_scheduled_backup_test.go | 13 ++++++-- pkg/jobs/BUILD.bazel | 1 + pkg/jobs/executor_impl_test.go | 9 +++-- pkg/jobs/job_scheduler_test.go | 26 +++++++++++---- pkg/jobs/scheduled_job.go | 16 +++++++-- pkg/jobs/utils.go | 33 +++++++++++++++++++ 7 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 pkg/jobs/utils.go diff --git a/pkg/ccl/backupccl/create_scheduled_backup.go b/pkg/ccl/backupccl/create_scheduled_backup.go index 1952dcea46d0..a6159edbe1cf 100644 --- a/pkg/ccl/backupccl/create_scheduled_backup.go +++ b/pkg/ccl/backupccl/create_scheduled_backup.go @@ -154,8 +154,15 @@ func computeScheduleRecurrence( `error parsing schedule expression: %q; it must be a valid cron expression`, cron) } - nextRun := expr.Next(now) - frequency := expr.Next(nextRun).Sub(nextRun) + nextRun, err := jobs.CronParseNext(expr, now, cron) + if err != nil { + return nil, err + } + nextToNextRun, err := jobs.CronParseNext(expr, nextRun, cron) + if err != nil { + return nil, err + } + frequency := nextToNextRun.Sub(nextRun) return &scheduleRecurrence{cron, frequency}, nil } diff --git a/pkg/ccl/backupccl/create_scheduled_backup_test.go b/pkg/ccl/backupccl/create_scheduled_backup_test.go index 89e45f845edf..65cf05ed4efc 100644 --- a/pkg/ccl/backupccl/create_scheduled_backup_test.go +++ b/pkg/ccl/backupccl/create_scheduled_backup_test.go @@ -389,6 +389,12 @@ func TestSerializesScheduledBackupExecutionArgs(t *testing.T) { query: `CREATE SCHEDULE FOR BACKUP INTO 'foo' WITH encryption_passphrase=$1 RECURRING '@hourly'`, errMsg: "failed to evaluate backup encryption_passphrase", }, + { + name: "malformed-cron-expression", + query: "CREATE SCHEDULE backup_schedule FOR BACKUP INTO 'userfile:///a' RECURRING '* 12-8 * * *';", + user: freeUser, + errMsg: "pq: failed to parse cron expression: \"\\* 12-8 \\* \\* \\*\"", + }, } for _, tc := range testCases { @@ -826,9 +832,10 @@ INSERT INTO t values (1), (10), (100); // to the next scheduled recurrence. for _, id := range []int64{fullID, incID} { s := loadSchedule(t, id) - require.EqualValues(t, - cronexpr.MustParse(s.ScheduleExpr()).Next(th.env.Now()).Round(time.Microsecond), - s.NextRun()) + expr := cronexpr.MustParse(s.ScheduleExpr()) + nextRun, err := jobs.CronParseNext(expr, th.env.Now(), s.ScheduleExpr()) + require.NoError(t, err) + require.EqualValues(t, nextRun.Round(time.Microsecond), s.NextRun()) } // We expect that, eventually, both backups would succeed. diff --git a/pkg/jobs/BUILD.bazel b/pkg/jobs/BUILD.bazel index 5a2279e60f1d..bd75db20086c 100644 --- a/pkg/jobs/BUILD.bazel +++ b/pkg/jobs/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "scheduled_job_executor.go", "testing_knobs.go", "update.go", + "utils.go", "validate.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/jobs", diff --git a/pkg/jobs/executor_impl_test.go b/pkg/jobs/executor_impl_test.go index 42a54e86cf13..8b7af3a15ad9 100644 --- a/pkg/jobs/executor_impl_test.go +++ b/pkg/jobs/executor_impl_test.go @@ -34,8 +34,13 @@ func TestInlineExecutorFailedJobsHandling(t *testing.T) { expectedNextRun time.Time }{ { - onError: jobspb.ScheduleDetails_RETRY_SCHED, - expectedNextRun: cronexpr.MustParse("@daily").Next(h.env.Now()).Round(time.Microsecond), + onError: jobspb.ScheduleDetails_RETRY_SCHED, + expectedNextRun: func() time.Time { + expr := cronexpr.MustParse("@daily") + nextRun, err := CronParseNext(expr, h.env.Now(), "@daily") + require.NoError(t, err) + return nextRun.Round(time.Microsecond) + }(), }, { onError: jobspb.ScheduleDetails_RETRY_SOON, diff --git a/pkg/jobs/job_scheduler_test.go b/pkg/jobs/job_scheduler_test.go index e08f7c8e7a42..288d80562224 100644 --- a/pkg/jobs/job_scheduler_test.go +++ b/pkg/jobs/job_scheduler_test.go @@ -74,7 +74,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) { })) // Verify the job has expected nextRun time. - expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now()) loaded := h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) @@ -92,7 +92,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) { if wait == jobspb.ScheduleDetails_WAIT { expectedRunTime = h.env.Now().Add(recheckRunningAfter) } else { - expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now()) } loaded = h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) @@ -132,7 +132,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) { })) // Verify the job has expected nextRun time. - expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now()) loaded := h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) @@ -146,7 +146,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) { return s.executeSchedules(ctx, allSchedules, txn) })) - expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now()) loaded = h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) }) @@ -172,7 +172,7 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) { })) // Verify the job has expected nextRun time. - expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now()) loaded := h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) @@ -186,11 +186,19 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) { return s.executeSchedules(ctx, allSchedules, txn) })) - expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now()) + expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now()) loaded = h.loadSchedule(t, j.ScheduleID()) require.Equal(t, expectedRunTime, loaded.NextRun()) } +func getNextRunTime(t *testing.T, cron string, fromNow time.Time) time.Time { + t.Helper() + expr := cronexpr.MustParse(cron) + nextRun, err := CronParseNext(expr, fromNow, cron) + require.NoError(t, err) + return nextRun +} + func TestJobSchedulerDaemonInitialScanDelay(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -534,7 +542,11 @@ func TestJobSchedulerRetriesFailed(t *testing.T) { }{ {jobspb.ScheduleDetails_PAUSE_SCHED, time.Time{}}, {jobspb.ScheduleDetails_RETRY_SOON, execTime.Add(retryFailedJobAfter).Round(time.Microsecond)}, - {jobspb.ScheduleDetails_RETRY_SCHED, cron.Next(execTime).Round(time.Microsecond)}, + {jobspb.ScheduleDetails_RETRY_SCHED, func() time.Time { + nextRun, err := CronParseNext(cron, execTime, "@hourly") + require.NoError(t, err) + return nextRun.Round(time.Microsecond) + }()}, } { t.Run(tc.onError.String(), func(t *testing.T) { h.env.SetTime(startTime) diff --git a/pkg/jobs/scheduled_job.go b/pkg/jobs/scheduled_job.go index 0ea20f5c110f..2657bcbb695f 100644 --- a/pkg/jobs/scheduled_job.go +++ b/pkg/jobs/scheduled_job.go @@ -183,8 +183,14 @@ func (j *ScheduledJob) Frequency() (time.Duration, error) { "parsing schedule expression: %q; it must be a valid cron expression", j.rec.ScheduleExpr) } - next := expr.Next(j.env.Now()) - nextNext := expr.Next(next) + next, err := CronParseNext(expr, j.env.Now(), j.rec.ScheduleExpr) + if err != nil { + return 0, err + } + nextNext, err := CronParseNext(expr, next, j.rec.ScheduleExpr) + if err != nil { + return 0, err + } return nextNext.Sub(next), nil } @@ -198,7 +204,11 @@ func (j *ScheduledJob) ScheduleNextRun() error { if err != nil { return errors.Wrapf(err, "parsing schedule expression: %q", j.rec.ScheduleExpr) } - j.SetNextRun(expr.Next(j.env.Now())) + nextRun, err := CronParseNext(expr, j.env.Now(), j.rec.ScheduleExpr) + if err != nil { + return err + } + j.SetNextRun(nextRun) return nil } diff --git a/pkg/jobs/utils.go b/pkg/jobs/utils.go new file mode 100644 index 000000000000..821991084052 --- /dev/null +++ b/pkg/jobs/utils.go @@ -0,0 +1,33 @@ +// Copyright 2021 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package jobs + +import ( + "time" + + "github.com/cockroachdb/errors" + "github.com/gorhill/cronexpr" +) + +// CronParseNext is a helper that parses the next time for the given expression +// but captures any panic that may occur in the underlying library. +func CronParseNext( + e *cronexpr.Expression, fromTime time.Time, cron string, +) (t time.Time, err error) { + defer func() { + if recover() != nil { + t = time.Time{} + err = errors.Newf("failed to parse cron expression: %q", cron) + } + }() + + return e.Next(fromTime), nil +}