Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: invalid pointer deref in pgotel instrumentation #1990

Merged

Conversation

fernandez14
Copy link
Contributor

@fernandez14 fernandez14 commented Nov 21, 2023

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

@fernandez14 fernandez14 changed the title fix: invalid pointer deref accessing results fix: invalid pointer deref in pgotel instrumentation Nov 21, 2023
@elliotcourant
Copy link
Collaborator

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
Copy link
Contributor Author

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 elliotcourant merged commit cfb7d2d into go-pg:v10 Nov 22, 2023
3 checks passed
@fernandez14 fernandez14 deleted the fix-invalid-rowsaffected-pointer-deref branch November 22, 2023 00:32
@elliotcourant
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants