Skip to content

Commit

Permalink
autoupdater: fix: nil pointer dereference on non status graphQL error
Browse files Browse the repository at this point in the history
If a GitHub GraphQL endpoint returned an error that is not an HTTP status error,
wrapGraphQLRetryableErrors() returned nil instead of the passed error.

This caused that Client.ReadyForMergeStatus() returned (nil,nil) and a nil
pointer deference was done in the autoupdater.

This fixes:
	panic: runtime error: invalid memory address or nil pointer dereference
	[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9293f6]
	goroutine 40 [running]:
	github.com/simplesurance/goordinator/internal/autoupdate.(*queue).updatePR(0xc0001e68f0, {0xc5b498, 0xc0000ae500}, 0xc00055e0a0)
	        github.com/simplesurance/goordinator/internal/autoupdate/queue.go:500 +0x476
	github.com/simplesurance/goordinator/internal/autoupdate.(*queue).scheduleUpdate.func1()
	        github.com/simplesurance/goordinator/internal/autoupdate/queue.go:422 +0xd4
	github.com/simplesurance/goordinator/internal/autoupdate/routines.(*Pool).worker(0xc00042eea0)
	        github.com/simplesurance/goordinator/internal/autoupdate/routines/pool.go:92 +0x75
	created by github.com/simplesurance/goordinator/internal/autoupdate/routines.NewPool
	        github.com/simplesurance/goordinator/internal/autoupdate/routines/pool.go:39 +0x133
  • Loading branch information
fho committed Mar 29, 2022
1 parent 6e9a214 commit 87855c4
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 2 deletions.
2 changes: 1 addition & 1 deletion internal/autoupdate/queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,8 @@ func (q *queue) updatePR(ctx context.Context, pr *PullRequest) {
logfields.ReviewDecision(status.ReviewDecision),
logfields.StatusCheckRollupState(status.StatusCheckRollupState),
)
if status.ReviewDecision != githubclt.ReviewDecisionApproved {

if status.ReviewDecision != githubclt.ReviewDecisionApproved {
if err := q.Suspend(pr.Number); err != nil {
logger.Error(
"suspending PR because it is not approved, failed",
Expand Down
2 changes: 1 addition & 1 deletion internal/githubclt/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ var graphQlHTTPStatusErrRe = regexp.MustCompile(`^non-200 OK status code: ([0-9]
func (clt *Client) wrapGraphQLRetryableErrors(err error) error {
matches := graphQlHTTPStatusErrRe.FindStringSubmatch(err.Error())
if len(matches) != 2 {
return nil
return err
}

errcode, atoiErr := strconv.Atoi(matches[1])
Expand Down
7 changes: 7 additions & 0 deletions internal/githubclt/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package githubclt

import (
"context"
"errors"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -37,3 +38,9 @@ func TestWrapRetryableErrorsGraphql(t *testing.T) {
var retryableErr *goorderr.RetryableError
assert.ErrorAs(t, err, &retryableErr)
}

func TestWrapRetryableErrorsGraphqlWithNonStatusErr(t *testing.T) {
err := errors.New("error")
wrappedErr := (&Client{}).wrapGraphQLRetryableErrors(err)
assert.Equal(t, err, wrappedErr)
}

0 comments on commit 87855c4

Please sign in to comment.