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

) (#23294) Backport #23216 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: Jason Song <i@wolfogre.com> Co-authored-by: delvh <dev.lh@web.de>
go-gitea · Mar 4, 2023 · a4158d1 · a4158d1
1 parent 7810192
commit a4158d1
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 14 deletions.
diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go
@@ -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
 	}
 

diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go
@@ -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"
@@ -207,15 +208,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{}{})
 }
 
@@ -248,16 +252,20 @@ 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 {
 		ctx.Error(http.StatusInternalServerError, err.Error())
 		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{}{})
 }
 

diff --git a/services/actions/clear_tasks.go b/services/actions/clear_tasks.go
@@ -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

diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go
@@ -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)

diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go
@@ -180,7 +180,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
 				}
 			}
 		}