Nil dereference panic in helper.Fail500

https://sentry.gitlap.com/gitlab/gitlab-workhorse-gitlabcom/issues/18394/

Missing nil check for w http.ResponseWriter. This is not critical for the user because the request has already failed by this time, but we should fix this. Maybe add a test for this method that feeds in nil values.

mentioned in issue #102 (closed)

changed milestone to %8.17

added Accepting Merge Requests label

added ~133901 label

assigned to @zj

Moving to %9.1, because %8.17 has passed!

changed milestone to %9.1

@jacobvosmaer-gitlab Sorry I started this late. Didn't find time earlier.

I have a question on your issue description however. To me it looks like the writer isn't the dereferenced value, the line number suggests the p or error itself is nil or dereferenced?

Reading the issue body, I feel like you suggest that this should do?

return http.HandlerFunc(raven.RecoveryHandler(
		func(w http.ResponseWriter, r *http.Request) {
			defer func() {
				if p := recover(); p != nil {
					helper.CleanHeadersForRaven(r)
					panic(p)
				}
			}()

			if w == nil {
				return
			}

			h.ServeHTTP(w, r)
		}))

The sentry stack trace shows:

*errors.errorString
runtime error: invalid memory address or nil pointer dereference
/usr/local/go/src/runtime/panic.go in gopanic at line 423
/var/cache/omnibus/src/gitlab-workhorse/_build/src/gitlab.com/gitlab-org/gitlab-workhorse/raven.go in 1 at line 29
/usr/local/go/src/runtime/asm_amd64.s in call32 at line 437
/usr/local/go/src/runtime/panic.go in gopanic at line 423
/usr/local/go/src/runtime/panic.go in panicmem at line 42
/usr/local/go/src/runtime/sigpanic_unix.go in sigpanic at line 24
… /src/gitlab-workhorse/_build/src/gitlab.com/gitlab-org/gitlab-workhorse/internal/helper/raven.go in captureRavenError at line 26
… rc/gitlab-workhorse/_build/src/gitlab.com/gitlab-org/gitlab-workhorse/internal/helper/helpers.go in Fail500 at line 20

@zj I find this hard to read but you have not convinced me the problem is not in helper.Fail500. Have you looked what the stack trace looks like if you put helper.Fail500 somewhere in an http handler that gets called by a test? Does the stack trace look similar?

Just hit this again with another customer https://gitlab.com/gitlab-org/gitlab-ce/snippets/1656840 - During a git push - https://gitlab.zendesk.com/agent/tickets/71598

added SP2 label

Isn't the problem here that err is nil, and we're attempting to call err.Error()?

func captureRavenError(r *http.Request, err error) {
    client := raven.DefaultClient

    interfaces := []raven.Interface{}
    if r != nil {
        CleanHeadersForRaven(r)
        interfaces = append(interfaces, raven.NewHttp(r))
    }

    exception := &raven.Exception{
        Stacktrace: raven.NewStacktrace(2, 3, nil),
        Value:      err.Error(), <--- here
        Type:       reflect.TypeOf(err).String(),
    }

added ~150846 label

mentioned in issue gitlab-com/organization#66 (closed)

removed assignee

Here's the code in v1.3.0 that caused the panic: https://gitlab.com/gitlab-org/gitlab-workhorse/blob/v1.3.0/internal/git/upload-pack.go#L75 - we were passing nil for both r and err, but either would be a problem.

This has been refactored out in the meantime: https://gitlab.com/gitlab-org/gitlab-workhorse/commit/47edcb1137e474cc68d60d13468b9346a0a394d1

So this specific error site doesn't exist any more. I've performed an audit of all Fail500 calls and don't see any that pass a nil for err or r any more.

So I don't think there's anything left to do here for master. We might want to consider backporting a fix to v1.3 if the relevant releases are still in support. Wdyt @jacobvosmaer-gitlab ?

@MrChrisW your customer was using the following:

GitLab 8.17.4-ee
GitLab Workhorse v1.3.0

Is an upgrade to 9.0 feasible for them? Would a backported fix help here?

assigned to @nick.thomas

closed

@nick.thomas I think Fail500 and LogError should gracefully handle nil values. Do they in master?

reopened

@jacobvosmaer-gitlab they are not. I'll submit an MR.

mentioned in merge request !156 (merged)

@nick.thomas My customer is running the GitLab for Pivotal Cloud Foundry http://docs.pivotal.io/partners/gitlab/ - Which means an upgrade to 9.0 is not possible a backport may help but we would still need to re-release pivotal. @WarheadsSE Any suggestions here?

https://gitlab.com/gitlab-pivotal/p-gitlab-ee/issues/24

closed via merge request !156 (merged)

@MrChrisW PCF will be getting 9.0 as 9.1 comes out.

/cc @nick.thomas

mentioned in commit tmaczukin/gitlab-workhorse@c03b113f

mentioned in commit tmaczukin/gitlab-workhorse@c03b113f

https://na34.salesforce.com/0016100000W3Iz5 this customer is reporting this and is using Pivotal Tile.

@MrChrisW we're prepared for PCF v1.2.0 with GitLab 9.0 in the coming days.

v1.2.0 of the PCF tile with GitLab 9.0.7 was released along with the rest of the security release versions. Thus, this should not longer be viable in up-to-date versions of the PCF tile.

mentioned in issue gitlab-com/organization#86