Skip to content

Commit d9b1635

Browse files
authored
[telemetry] fix segfault (#1569)
## Summary `newSentryException` can error [here](https://github.com/jetpack-io/devbox/blob/main/internal/telemetry/sentry.go#L55) due to `err` being `nil`. Its only callsite is [here](https://github.com/jetpack-io/devbox/blob/main/internal/telemetry/telemetry.go#L130), 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.
1 parent bb5b0aa commit d9b1635

File tree

3 files changed

+28
-8
lines changed

3 files changed

+28
-8
lines changed

internal/telemetry/sentry.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ func initSentryClient(appName string) bool {
5151
return err == nil
5252
}
5353

54-
func newSentryException(err error) []sentry.Exception {
55-
errMsg := err.Error()
54+
func newSentryException(errToLog error) []sentry.Exception {
55+
errMsg := errToLog.Error()
5656
binPkg := ""
5757
modPath := ""
5858
if build, ok := debug.ReadBuildInfo(); ok {
@@ -66,12 +66,12 @@ func newSentryException(err error) []sentry.Exception {
6666
var stFunc func() []runtime.Frame
6767
errType := "Generic Error"
6868
for {
69-
if t := exportedErrType(err); t != "" {
69+
if t := exportedErrType(errToLog); t != "" {
7070
errType = t
7171
}
7272

7373
//nolint:errorlint
74-
switch stackErr := err.(type) {
74+
switch stackErr := errToLog.(type) {
7575
// If the error implements the StackTrace method in the redact package, then
7676
// prefer that. The Sentry SDK gets some things wrong when guessing how
7777
// to extract the stack trace.
@@ -98,11 +98,11 @@ func newSentryException(err error) []sentry.Exception {
9898
return frames
9999
}
100100
}
101-
uw := errors.Unwrap(err)
101+
uw := errors.Unwrap(errToLog)
102102
if uw == nil {
103103
break
104104
}
105-
err = uw
105+
errToLog = uw
106106
}
107107
ex := []sentry.Exception{{Type: errType, Value: errMsg}}
108108
if stFunc != nil {

internal/telemetry/telemetry.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ func commandEvent(meta Metadata) (id string, msg *segment.Track) {
114114

115115
// Error reports an error to the telemetry server.
116116
func Error(err error, meta Metadata) {
117-
if !started || err == nil {
117+
errToLog := err // use errToLog to avoid shadowing err later. Use err to keep API clean.
118+
if !started || errToLog == nil {
118119
return
119120
}
120121

@@ -127,7 +128,7 @@ func Error(err error, meta Metadata) {
127128
EventID: sentry.EventID(ExecutionID),
128129
Level: sentry.LevelError,
129130
User: sentry.User{ID: deviceID},
130-
Exception: newSentryException(redact.Error(err)),
131+
Exception: newSentryException(redact.Error(errToLog)),
131132
Contexts: map[string]map[string]any{
132133
"os": {
133134
"name": build.OS(),

internal/telemetry/telemetry_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package telemetry
2+
3+
import (
4+
"errors"
5+
"testing"
6+
)
7+
8+
// TestErrorBasic does a very simple sanity check to ensure the error can be sent
9+
// to the sentry and segment buffers
10+
func TestErrorBasic(t *testing.T) {
11+
segmentBufferDir = t.TempDir()
12+
sentryBufferDir = t.TempDir()
13+
started = true
14+
15+
fakeErr := errors.New("fake error")
16+
meta := Metadata{}
17+
18+
Error(fakeErr, meta)
19+
}

0 commit comments

Comments
 (0)