panic: assignment to entry in nil map when using non-default Transports

[RaghavSood]

Summary

We're facing an issue packaging https://github.com/superfly/flyctl for nixpkgs due to the way they initialise the sentry SDK.

Their init function defines a non-default HTTPSyncTransport. However, they only provide the Timeout parameter, which leaves the rest as nil, including the non-exported limits.

This eventually results in a panic when the limits map is merged with the response headers:

github.com/getsentry/sentry-go/internal/ratelimit.Map.Merge(...)
	/Users/raghavsood/r/local/dev/github.com/superfly/flyctl/.devenv/state/go/pkg/mod/github.com/getsentry/sentry-go@v0.28.0/internal/ratelimit/map.go:43

This can be reproduced by running:

go test -tags=production -ldflags "-s -w -X github.com/superfly/flyctl/internal/buildinfo.buildDate=1970-01-01T00:00:00Z -X github.com/superfly/flyctl/internal/buildinfo.buildVersion=0.2.71" -v ./internal/command/deploy/deploy_test.go 

against flyctl v0.2.71 on at least darwin m1/x86 (I can't speak to how they use sentry on other platforms)

This can also be replicated with the testing suit by applying this:

diff --git a/internal/ratelimit/map_test.go b/internal/ratelimit/map_test.go
index 4e6b555..9744dac 100644
--- a/internal/ratelimit/map_test.go
+++ b/internal/ratelimit/map_test.go
@@ -256,6 +256,18 @@ func TestMapMerge(t *testing.T) {
                        new:  Map{CategoryError: Deadline(now.Add(time.Minute))},
                        want: Map{CategoryError: Deadline(now.Add(time.Minute))},
                },
+               {
+                       name: "nil old inits a default map",
+                       old:  nil,
+                       new:  Map{},
+                       want: Map{},
+               },
+               {
+                       name: "nil old is merged with new",
+                       old:  nil,
+                       new:  Map{CategoryError: Deadline(now)},
+                       want: Map{CategoryError: Deadline(now)},
+               },
        }
        for _, tt := range tests {
                tt := tt

Since map.Merge is a value receiver, not a pointer receiver, it is not possible to handle this and init a new map within the Merge itself.

Steps To Reproduce

Apply this patch to the test files:

diff --git a/internal/ratelimit/map_test.go b/internal/ratelimit/map_test.go
index 4e6b555..9744dac 100644
--- a/internal/ratelimit/map_test.go
+++ b/internal/ratelimit/map_test.go
@@ -256,6 +256,18 @@ func TestMapMerge(t *testing.T) {
                        new:  Map{CategoryError: Deadline(now.Add(time.Minute))},
                        want: Map{CategoryError: Deadline(now.Add(time.Minute))},
                },
+               {
+                       name: "nil old inits a default map",
+                       old:  nil,
+                       new:  Map{},
+                       want: Map{},
+               },
+               {
+                       name: "nil old is merged with new",
+                       old:  nil,
+                       new:  Map{CategoryError: Deadline(now)},
+                       want: Map{CategoryError: Deadline(now)},
+               },
        }
        for _, tt := range tests {
                tt := tt

Then run:

go test  ./internal/ratelimit/...

Expected Behavior

Sentry should either not allow a nil limits to occur, or set a sane default within the transport/client call chain

SDK

• sentry-go version: v0.28.0
• Go version: go1.22.3
• Using Go Modules? yes

[ribice]

I tried downgrading Sentry to 0.27.0 and upgrading it to 0.28.1 - both have the same issue. Will investigate further.

[RaghavSood]

It has likely been a latent issue for some time now that just surfaced accidentally with flyctl here.

I was able to get it to work as expected with a simple nil check before merge:

diff --git a/transport.go b/transport.go
index 20f6994..4dc50d6 100644
--- a/transport.go
+++ b/transport.go
@@ -690,6 +690,9 @@ func (t *HTTPSyncTransport) SendEventWithContext(ctx context.Context, event *Eve
        }
 
        t.mu.Lock()
+       if t.limits == nil {
+               t.limits = make(ratelimit.Map)
+       }
        t.limits.Merge(ratelimit.FromResponse(response))
        t.mu.Unlock() 

For now, I've proposed an upstream fix to set the timeout on top of the default client provided by the go-sdk: superfly/flyctl#3643

This kind of transport initialization only seems to be used by a few consumers, who may not be hitting other conditions required to get the panic (maybe they never end up rate limited?):

https://github.com/search?q=%26sentry.HTTPSyncTransport&type=code
https://github.com/search?q=%26sentry.HTTPTransport&type=code

Which would help explain why it went unreported so far