Nulo/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Avoid panic caused by broken payload when creating commit status (#23216)

Browse source 
When creating commit status for Actons jobs, a payload with nil
`HeadCommit` will cause panic.

Reported at:
https://gitea.com/gitea/act_runner/issues/28#issuecomment-732166

Although the `HeadCommit` probably can not be nil after #23215,
`CreateCommitStatus` should protect itself, to avoid being broken in the
future.

In addition, it's enough to print error log instead of returning err
when `CreateCommitStatus` failed.

---------

Co-authored-by: delvh <dev.lh@web.de>
This commit is contained in:
Jason Song 2023-03-04 15:12:37 +08:00 committed by GitHub
parent ca84a61761
commit 47b912cd52
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 45 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
routers/api/actions/runner/runner.go
View file

@ -150,7 +150,7 @@ func (s *Service) UpdateTask(
}
if err := actions_service.CreateCommitStatus(ctx, task.Job); err != nil {
log.Error("Update commit status failed: %v", err)
log.Error("Update commit status for job %v failed: %v", task.Job.ID, err)
// go on
}

22
routers/web/repo/actions/view.go
View file

@ -15,6 +15,7 @@ import (
"code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/modules/actions"
context_module "code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
@ -214,15 +215,18 @@ func Rerun(ctx *context_module.Context) {
job.Stopped = 0
if err := db.WithTx(ctx, func(ctx context.Context) error {
if _, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped"); err != nil {
return err
}
return actions_service.CreateCommitStatus(ctx, job)
_, err := actions_model.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped")
return err
}); err != nil {
ctx.Error(http.StatusInternalServerError, err.Error())
return
}
if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
ctx.JSON(http.StatusOK, struct{}{})
}
@ -255,9 +259,6 @@ func Cancel(ctx *context_module.Context) {
if err := actions_model.StopTask(ctx, job.TaskID, actions_model.StatusCancelled); err != nil {
return err
}
if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
return err
}
}
return nil
}); err != nil {
@ -265,6 +266,13 @@ func Cancel(ctx *context_module.Context) {
return
}
for _, job := range jobs {
if err := actions_service.CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}
ctx.JSON(http.StatusOK, struct{}{})
}

22
services/actions/clear_tasks.go
View file

@ -43,6 +43,7 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
return fmt.Errorf("find tasks: %w", err)
}
jobs := make([]*actions_model.ActionRunJob, 0, len(tasks))
for _, task := range tasks {
if err := db.WithTx(ctx, func(ctx context.Context) error {
if err := actions_model.StopTask(ctx, task.ID, actions_model.StatusFailure); err != nil {
@ -51,7 +52,8 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
if err := task.LoadJob(ctx); err != nil {
return err
}
return CreateCommitStatus(ctx, task.Job)
jobs = append(jobs, task.Job)
return nil
}); err != nil {
log.Warn("Cannot stop task %v: %v", task.ID, err)
// go on
@ -61,6 +63,14 @@ func stopTasks(ctx context.Context, opts actions_model.FindTaskOptions) error {
remove()
}
}
for _, job := range jobs {
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}
return nil
}
@ -80,14 +90,16 @@ func CancelAbandonedJobs(ctx context.Context) error {
job.Status = actions_model.StatusCancelled
job.Stopped = now
if err := db.WithTx(ctx, func(ctx context.Context) error {
if _, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped"); err != nil {
return err
}
return CreateCommitStatus(ctx, job)
_, err := actions_model.UpdateRunJob(ctx, job, nil, "status", "stopped")
return err
}); err != nil {
log.Warn("cancel abandoned job %v: %v", job.ID, err)
// go on
}
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}
return nil

10
services/actions/commit_status.go
View file

@ -30,6 +30,16 @@ func CreateCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er
return fmt.Errorf("GetPushEventPayload: %w", err)
}
// Since the payload comes from json data, we should check if it's broken, or it will cause panic
switch {
case payload.Repo == nil:
return fmt.Errorf("repo is missing in event payload")
case payload.Pusher == nil:
return fmt.Errorf("pusher is missing in event payload")
case payload.HeadCommit == nil:
return fmt.Errorf("head commit is missing in event payload")
}
creator, err := user_model.GetUserByID(ctx, payload.Pusher.ID)
if err != nil {
return fmt.Errorf("GetUserByID: %w", err)

3
services/actions/notifier_helper.go
View file

@ -187,7 +187,8 @@ func notify(ctx context.Context, input *notifyInput) error {
} else {
for _, job := range jobs {
if err := CreateCommitStatus(ctx, job); err != nil {
log.Error("CreateCommitStatus: %v", err)
log.Error("Update commit status for job %v failed: %v", job.ID, err)
// go on
}
}
}