go-pg / pg

Golang ORM with focus on PostgreSQL features and performance
https://pg.uptrace.dev/
BSD 2-Clause "Simplified" License
5.65k stars 401 forks source link

fix: invalid pointer deref in pgotel instrumentation #1990

Closed fernandez14 closed 9 months ago

fernandez14 commented 9 months ago

We've been experiencing panics on production code with a stack trace like this:

panic runtime error: invalid memory address or nil pointer dereference [recovered] 
    go.opentelemetry.io/otel/sdk/trace/span.go:383 (*recordingSpan).End.func1
    go.opentelemetry.io/otel/sdk/trace/span.go:421 (*recordingSpan).End
    /usr/local/go/src/runtime/panic.go:914 panic
    github.com/go-pg/pg/v10/result.go:48 (*result).RowsAffected
    github.com/go-pg/pg/extra/pgotel/v10/pgotel.go:129 (*TracingHook).AfterQuery
    github.com/go-pg/pg/v10/hook.go:130 (*baseDB).afterQueryFromIndex
    github.com/go-pg/pg/v10/hook.go:125 (*baseDB).afterQuery
    github.com/go-pg/pg/v10/tx.go:238 (*Tx).query
    github.com/go-pg/pg/v10/tx.go:211 (*Tx).QueryContext
    github.com/go-pg/pg/v10/orm/query.go:1163 (*Query).returningQuery
    github.com/go-pg/pg/v10/orm/query.go:1034 (*Query).Insert

We noticed that, while there's a nil guard on evt.Result, we also need to check that, when result implementation is filled with a pointer that pointer is not nil.

I've written a similar example of this issue on https://go.dev/play/p/mVBQkMv7lJU

elliotcourant commented 9 months ago

Would you be able to add a test similar to the one you have in that go playground as part of this PR? That way it's easier to make sure this doesn't break again in the future

fernandez14 commented 9 months ago

Would you be able to add a test similar to the one you have in that go playground as part of this PR? That way it's easier to make sure this doesn't break again in the future

I isolated the check into a function and added a test to it. Let me know how it looks and thanks for taking a look.

elliotcourant commented 8 months ago

@fernandez14 sorry for the delay but your change is now tagged in v10.12.0. Thank you again!