Skip to content

[telemetry] fix segfault #1569

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

Merged
merged 2 commits into from
Oct 18, 2023
Merged

[telemetry] fix segfault #1569

merged 2 commits into from
Oct 18, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Oct 18, 2023

Summary

newSentryException can error here due to err being nil. Its only callsite is here, but the nix.Version above is resetting the err to nil leading to a segfault.

This was my bad from #1469

In this PR, I change the variable err to errToLog so that any of us adding other code here do not accidentally clobber the errToLog variable.

I think but not sure this is the cause of #1562 (but not sure).

How was it tested?

I managed to get a repro with the nginx example and running devbox version 0.6, and doing devbox services up -b, when the services are already running leading to an error.

When I run it with the new devbox binary, it errors correctly without a segfault.

Copy link
Collaborator Author

savil commented Oct 18, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested a review from gcurtis October 18, 2023 00:49
@savil savil marked this pull request as ready for review October 18, 2023 00:50
Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test for Error for this? It would just need to set segmentBufferDir and sentryBufferDir to a temp dir so that they don't actually get sent to Segment/Sentry. I should've added some basic tests back when this was created.

@@ -113,8 +113,8 @@ func commandEvent(meta Metadata) (id string, msg *segment.Track) {
}

// Error reports an error to the telemetry server.
func Error(err error, meta Metadata) {
if !started || err == nil {
func Error(errToLog error, meta Metadata) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we leave this one as err just to keep the package API clean?

@savil
Copy link
Collaborator Author

savil commented Oct 18, 2023

@gcurtis good call. I added a very very basic test that would fail without this PR's code changes.

@savil savil merged commit d9b1635 into main Oct 18, 2023
@savil savil deleted the savil/telemetry-error-segfault branch October 18, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants