Skip to content

Add CGo-safe JSON methods and wrap API methods with RunOffCgoStack#363

Merged
myleshorton merged 3 commits intomainfrom
fix/split-tunnel-cgo-safe-json
Mar 23, 2026
Merged

Add CGo-safe JSON methods and wrap API methods with RunOffCgoStack#363
myleshorton merged 3 commits intomainfrom
fix/split-tunnel-cgo-safe-json

Conversation

@myleshorton
Copy link
Contributor

@myleshorton myleshorton commented Mar 20, 2026

Summary

  • Adds Filter.Items(filterType) method to consolidate the read-side filter type switch
  • Adds SplitTunnel.ItemsJSON and SplitTunnel.EnabledAppsJSON with RunOffCgoStack
  • Wraps all []byte-returning APIClient methods with RunOffCgoStack: UserData, FetchUserData, Login, Logout, DeleteAccount, OAuthLoginCallback
  • Follows the ServersJSON() pattern: radiance owns data + marshaling on a real Go goroutine

Motivation

Gomobile-exported functions run on a CGo callback stack whose memory is not covered by the GC heap bitmap. When these methods allocate protobuf structs, maps, or slices and marshal them to []byte, bulkBarrierPreWrite panics when copying the return value across the C boundary.

Companion PR

Test plan

  • Existing tests pass
  • macOS no longer crashes on startup with split tunnel enabled
  • Login/logout/OAuth flows work on all platforms
  • Split tunnel UI loads correctly

Fixes getlantern/engineering#3088

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 20, 2026 13:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds CGo-safe JSON helper methods to vpn.SplitTunnel, aligning split-tunnel JSON access with the existing “pre-marshalled JSON” patterns used elsewhere in the codebase and moving marshaling closer to where the split tunnel types live.

Changes:

  • Add (*SplitTunnel).ItemsJSON(filterType) to return filter items as JSON via common.RunOffCgoStack.
  • Add (*SplitTunnel).EnabledAppsJSON() to return enabled app identifiers as JSON via common.RunOffCgoStack.
  • Disambiguate JSON packages by introducing encoding/json for simple marshaling and aliasing sing/common/json as singjson for rule set serialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Split tunnel:
- Add Filter.Items(filterType) to consolidate the read-side switch
- Add SplitTunnel.ItemsJSON and EnabledAppsJSON with RunOffCgoStack
- Use atomicfile.ReadFile, common.IsWindows(), handle marshal errors
- Add tests for both new methods

API client:
- Wrap UserData, FetchUserData, Login, Logout, DeleteAccount, and
  OAuthLoginCallback with RunOffCgoStack to avoid CGo write barrier
  panics when protobuf structs are allocated on the callback stack

Fixes getlantern/engineering#3088

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@myleshorton myleshorton force-pushed the fix/split-tunnel-cgo-safe-json branch from 564f040 to fb32d1c Compare March 20, 2026 14:23
@myleshorton myleshorton changed the title Add ItemsJSON and EnabledAppsJSON to SplitTunnel Add CGo-safe JSON methods and wrap API methods with RunOffCgoStack Mar 20, 2026
…ustness

- ItemsJSON: ensure non-nil slice so empty filters return [] not null
- EnabledAppsJSON: use singjson.UnmarshalExtended for legacy file parse
  (consistent with loadRule which also uses extended JSON)
- Windows dedup: lowercase only the dedup key, preserve original values
- Tests: unmarshal JSON to []string and assert on slices instead of
  brittle string matching

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Collaborator

@garmr-ulfr garmr-ulfr left a comment

Choose a reason for hiding this comment

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

The cgo part makes sense, but I'm confused on the need for any of the added functions. It would be much simpler to just marshal Filters and then lantern would just reference the fields.

@myleshorton
Copy link
Contributor Author

I think those are the ones I just moved from core.go in lantern to consolidate.

Per review: bundleId, bundleID, enabledApps, and apps were never valid
split-tunnel config keys. Only processPath, processPathRegex, and
packageName exist in legacy configs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@myleshorton
Copy link
Contributor Author

Call chain verification

Both methods added in this PR are called from the lantern app via companion PR lantern#8556, which replaces ~90 lines of duplicated logic with thin wrappers:

Radiance method Lantern-core wrapper FFI (desktop) Mobile (gomobile) Dart
ItemsJSON GetSplitTunnelItems getSplitTunnelItems (ffi.go:948) GetSplitTunnelItems (mobile.go:528) Both FFI + platform service
EnabledAppsJSON GetEnabledApps getEnabledApps (ffi.go:998) N/A (desktop-only feature) FFI service only

Both methods use RunOffCgoStack internally to avoid the CGo write barrier crashes that motivated this PR.

@myleshorton myleshorton merged commit 279e5ef into main Mar 23, 2026
2 checks passed
@myleshorton myleshorton deleted the fix/split-tunnel-cgo-safe-json branch March 23, 2026 18:47
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.

3 participants