Skip to content

Commit bf610b5

Browse files
myleshortonclaude
andauthored
Add CGo-safe JSON methods and RunOnGoStack helper (#361)
* Add CGo-safe JSON methods and RunOnGoStack helper When radiance types (Server, Options containing sing-box option.Outbound with `any` interfaces) are returned to callers on a CGo callback stack, the GC write barrier panics because the heap bitmap doesn't cover the C stack. This caused crashes in lantern's GetAvailableServers on macOS. Add ServersJSON() and GetServerByTagJSON() to Manager that marshal under the lock and return plain []byte, safe for CGo callers. Add a generic RunOnGoStack[T] helper in common/ for any remaining cases where callers need to move pointer-rich work off the CGo stack. Includes defer/recover so a panic returns an error instead of blocking forever. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address PR review: tests, docstring, marshal under lock - Add tests for ServersJSON and GetServerByTagJSON validating JSON output includes sing-box type-specific fields and location data - Fix GetServerByTagJSON to perform lookup + marshal under a single RLock using MarshalContext with the sing-box context - Fix RunOnGoStack docstring: clarify a new goroutine is spawned per call (no persistent worker) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Simplify: extract getServerByTagLocked, use MarshalContext in ServersJSON - Extract shared lookup logic into getServerByTagLocked to eliminate duplication between GetServerByTag and GetServerByTagJSON - Use json.MarshalContext in ServersJSON for consistency with GetServerByTagJSON (ensures sing-box type-specific fields are included) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove unused RunOnGoStack helper The CGo write barrier issue is fully addressed by ServersJSON() and GetServerByTagJSON(), which marshal pointer-rich sing-box types inside radiance under the read lock and return plain []byte. Since no pointer-rich Go values cross the CGo boundary, the generic goroutine helper is unnecessary. Removing to avoid dead code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Restore RunOffCgoStack helper in common/ Gomobile-exported functions run on a CGo callback stack whose memory isn't covered by the GC heap bitmap. The gomobile-generated wrapper copies Go pointer-containing return values to the C thread stack, which can cause bulkBarrierPreWrite panics. RunOffCgoStack runs the function body on a real Go goroutine to avoid this. Previously named RunOnGoStack, renamed to better describe the intent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Move RunOffCgoStack into ServersJSON and GetServerByTagJSON Callers no longer need to wrap these methods — the goroutine hop is handled internally so any caller (gomobile, FFI, or pure Go) is automatically safe from CGo write barrier panics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Document why GetServerByTag doesn't need RunOffCgoStack Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 725ad37 commit bf610b5

3 files changed

Lines changed: 176 additions & 1 deletion

File tree

common/gostack.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package common
2+
3+
import (
4+
"fmt"
5+
"log/slog"
6+
"runtime/debug"
7+
)
8+
9+
// RunOffCgoStack executes fn on a new goroutine and returns its result.
10+
// A new goroutine is spawned per call; there is no persistent worker.
11+
//
12+
// Gomobile-exported functions run on a CGo callback stack whose memory isn't
13+
// covered by the GC heap bitmap. When the gomobile-generated wrapper copies Go
14+
// pointer-containing return values to the C thread stack, bulkBarrierPreWrite
15+
// can panic. Running the body on a real Go goroutine avoids this entirely.
16+
//
17+
// If fn panics, the panic is recovered and a zero value + error are returned
18+
// instead of blocking the caller forever.
19+
func RunOffCgoStack[T any](fn func() (T, error)) (T, error) {
20+
type result struct {
21+
val T
22+
err error
23+
}
24+
ch := make(chan result, 1)
25+
go func() {
26+
defer func() {
27+
if r := recover(); r != nil {
28+
slog.Error("panic in RunOffCgoStack", "panic", r, "stack", string(debug.Stack()))
29+
var zero T
30+
ch <- result{val: zero, err: fmt.Errorf("panic: %v", r)}
31+
}
32+
}()
33+
v, err := fn()
34+
ch <- result{val: v, err: err}
35+
}()
36+
r := <-ch
37+
return r.val, r.err
38+
}

servers/manager.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,11 +182,18 @@ type Server struct {
182182
}
183183

184184
// GetServerByTag returns the server configuration for a given tag and a boolean indicating whether
185-
// the server was found.
185+
// the server was found. The returned Server contains pointer-rich sing-box types in its Options
186+
// field, so callers on a CGo callback stack should use [GetServerByTagJSON] instead. This method
187+
// does not use [common.RunOffCgoStack] because its only callers run on regular Go goroutines
188+
// (event subscribers, private server flows), never on CGo callback stacks.
186189
func (m *Manager) GetServerByTag(tag string) (Server, bool) {
187190
m.access.RLock()
188191
defer m.access.RUnlock()
192+
return m.getServerByTagLocked(tag)
193+
}
189194

195+
// getServerByTagLocked performs the tag lookup. Caller must hold access.RLock.
196+
func (m *Manager) getServerByTagLocked(tag string) (Server, bool) {
190197
group := SGLantern
191198
opts, ok := m.optsMaps[SGLantern][tag]
192199
if !ok {
@@ -210,6 +217,41 @@ func (m *Manager) GetServerByTag(tag string) (Server, bool) {
210217
return s, true
211218
}
212219

220+
// ServersJSON returns the current server configurations as pre-marshalled JSON.
221+
// Safe to call from CGo callback stacks: the work runs on a dedicated Go goroutine
222+
// (via [common.RunOffCgoStack]) so pointer-rich sing-box types never touch the C stack.
223+
func (m *Manager) ServersJSON() ([]byte, error) {
224+
return common.RunOffCgoStack(func() ([]byte, error) {
225+
m.access.RLock()
226+
defer m.access.RUnlock()
227+
return json.MarshalContext(box.BaseContext(), m.servers)
228+
})
229+
}
230+
231+
// GetServerByTagJSON returns the server configuration for a given tag as pre-marshalled JSON.
232+
// Like [ServersJSON], safe to call from CGo callback stacks.
233+
func (m *Manager) GetServerByTagJSON(tag string) ([]byte, bool, error) {
234+
type result struct {
235+
data []byte
236+
ok bool
237+
}
238+
r, err := common.RunOffCgoStack(func() (result, error) {
239+
m.access.RLock()
240+
defer m.access.RUnlock()
241+
242+
s, ok := m.getServerByTagLocked(tag)
243+
if !ok {
244+
return result{}, nil
245+
}
246+
b, err := json.MarshalContext(box.BaseContext(), s)
247+
if err != nil {
248+
return result{}, fmt.Errorf("marshal server %q: %w", tag, err)
249+
}
250+
return result{data: b, ok: true}, nil
251+
})
252+
return r.data, r.ok, err
253+
}
254+
213255
type ServersUpdatedEvent struct {
214256
events.Event
215257
Group ServerGroup

servers/manager_test.go

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,101 @@ import (
2222
"github.com/getlantern/radiance/common"
2323
)
2424

25+
func newTestManager(t *testing.T) *Manager {
26+
t.Helper()
27+
dataPath := t.TempDir()
28+
mgr := &Manager{
29+
servers: Servers{
30+
SGLantern: Options{
31+
Outbounds: []option.Outbound{
32+
{Tag: "ss-denver", Type: "shadowsocks", Options: &option.ShadowsocksOutboundOptions{
33+
ServerOptions: option.ServerOptions{
34+
Server: "1.2.3.4",
35+
ServerPort: 1080,
36+
},
37+
Method: "chacha20-ietf-poly1305",
38+
Password: "testpass",
39+
}},
40+
},
41+
Endpoints: make([]option.Endpoint, 0),
42+
Locations: map[string]C.ServerLocation{
43+
"ss-denver": {Country: "US", City: "Denver", CountryCode: "US"},
44+
},
45+
Credentials: make(map[string]ServerCredentials),
46+
},
47+
SGUser: Options{
48+
Outbounds: make([]option.Outbound, 0),
49+
Endpoints: make([]option.Endpoint, 0),
50+
Locations: make(map[string]C.ServerLocation),
51+
Credentials: make(map[string]ServerCredentials),
52+
},
53+
},
54+
optsMaps: map[ServerGroup]map[string]any{
55+
SGLantern: {"ss-denver": option.Outbound{Tag: "ss-denver", Type: "shadowsocks", Options: &option.ShadowsocksOutboundOptions{
56+
ServerOptions: option.ServerOptions{Server: "1.2.3.4", ServerPort: 1080},
57+
Method: "chacha20-ietf-poly1305",
58+
Password: "testpass",
59+
}}},
60+
SGUser: make(map[string]any),
61+
},
62+
serversFile: filepath.Join(dataPath, common.ServersFileName),
63+
}
64+
return mgr
65+
}
66+
67+
func TestServersJSON(t *testing.T) {
68+
mgr := newTestManager(t)
69+
70+
b, err := mgr.ServersJSON()
71+
require.NoError(t, err)
72+
require.NotEmpty(t, b)
73+
74+
// Must be valid JSON
75+
var raw map[string]json.RawMessage
76+
require.NoError(t, json.Unmarshal(b, &raw), "ServersJSON must return valid JSON")
77+
assert.Contains(t, raw, "lantern")
78+
assert.Contains(t, raw, "user")
79+
80+
// Lantern group must include the sing-box type-specific fields
81+
lanternJSON := string(raw["lantern"])
82+
assert.Contains(t, lanternJSON, "shadowsocks", "should contain outbound type")
83+
assert.Contains(t, lanternJSON, "1.2.3.4", "should contain server address")
84+
assert.Contains(t, lanternJSON, "1080", "should contain server port")
85+
assert.Contains(t, lanternJSON, "chacha20-ietf-poly1305", "should contain method")
86+
}
87+
88+
func TestGetServerByTagJSON(t *testing.T) {
89+
mgr := newTestManager(t)
90+
91+
t.Run("existing tag", func(t *testing.T) {
92+
b, ok, err := mgr.GetServerByTagJSON("ss-denver")
93+
require.NoError(t, err)
94+
require.True(t, ok)
95+
require.NotEmpty(t, b)
96+
97+
// Must be valid JSON
98+
var raw map[string]json.RawMessage
99+
require.NoError(t, json.Unmarshal(b, &raw), "GetServerByTagJSON must return valid JSON")
100+
assert.Contains(t, raw, "Tag")
101+
assert.Contains(t, raw, "Type")
102+
assert.Contains(t, raw, "Options")
103+
assert.Contains(t, raw, "Location")
104+
105+
// Verify the correct tag and type
106+
fullJSON := string(b)
107+
assert.Contains(t, fullJSON, "ss-denver")
108+
assert.Contains(t, fullJSON, "shadowsocks")
109+
assert.Contains(t, fullJSON, "Denver")
110+
})
111+
112+
t.Run("missing tag", func(t *testing.T) {
113+
b, ok, err := mgr.GetServerByTagJSON("nonexistent")
114+
assert.NoError(t, err)
115+
assert.False(t, ok)
116+
assert.Nil(t, b)
117+
})
118+
}
119+
25120
func TestPrivateServerIntegration(t *testing.T) {
26121
dataPath := t.TempDir()
27122
manager := &Manager{

0 commit comments

Comments
 (0)