From a95e01b3a95bba222cefde48a1546adfe451d427 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 16 Aug 2026 01:09:58 -0600 Subject: [PATCH 1/2] utils: make the gomobile message guarantee unconditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous pass returned the callee's error untouched whenever its message was already valid, on the assumption that the checked string was the one the bridge would marshal. It isn't. genobjc builds `@{NSLocalizedDescriptionKey: [self error]}`, and [self error] calls back into Go — so the bridge reads Error() again, and only a deterministic Error() answers the same thing twice. An error formatting a buffer another goroutine is still appending to, or holding a partially-read response body, could pass the check and still hand invalid UTF-8 to initWithBytesNoCopy, get nil, and abort on the dictionary insert. The check was advisory; the frozen message is what actually holds. Wrap unconditionally. Error() also runs on the caller's goroutine, outside the recover that guards fn, so an interface holding a typed nil pointer took the process down from inside the helper meant to prevent exactly that. Recover around it. The empty-message branch stays, but its comment was wrong: go_seq_to_objc_string returns @"" for a zero-length string and never reaches initWithBytesNoCopy, so an empty message cannot cause the abort. It is kept because a blank error is useless in a crash report, not because it is unsafe. Verified against the pinned gomobile rather than inferred: genobjc.go:928 for the second Error() call, seq_darwin.m.support:140 for the empty short-circuit. Co-Authored-By: Claude Opus 5 (1M context) --- lantern-core/utils/gostack.go | 45 +++++++++++++++++++----------- lantern-core/utils/gostack_test.go | 43 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/lantern-core/utils/gostack.go b/lantern-core/utils/gostack.go index 6465fe6d60..7564ddc558 100644 --- a/lantern-core/utils/gostack.go +++ b/lantern-core/utils/gostack.go @@ -19,16 +19,20 @@ import ( // If fn panics, the panic is recovered and a zero value + error are returned // instead of blocking the caller forever. // -// Returned errors are guaranteed to carry a non-empty, valid-UTF-8 message -// before crossing back into gomobile's -// objc bridge. The bridge wraps non-nil Go errors as a Universeerror whose -// initWithRef calls [NSString initWithBytesNoCopy: ... encoding:UTF8] on the -// raw error bytes; that returns nil for invalid UTF-8 (e.g. a gzipped 404 -// page, or a binary blob from an upstream LB), and the dictionary literal -// `@{NSLocalizedDescriptionKey: nil}` then aborts the app with -// "attempt to insert nil object from objects[0]". Sanitizing here means every -// gomobile-exported function that funnels through RunOffCgoStack is safe by -// construction, regardless of what shape of error its callee returns. +// Returned errors always carry a frozen, valid-UTF-8 message before crossing +// back into gomobile's objc bridge. The bridge wraps non-nil Go errors as a +// Universeerror whose initWithRef builds +// `@{NSLocalizedDescriptionKey: [self error]}`; [self error] calls back into +// Go, and the result reaches [NSString initWithBytesNoCopy: ... encoding:UTF8], +// which returns nil for invalid UTF-8 (a gzipped 404 page, a binary blob from +// an upstream LB). Inserting that nil into the dictionary literal aborts the +// app with "attempt to insert nil object from objects[0]". +// +// Because the bridge calls Error() itself rather than reusing anything checked +// here, validating a message and then returning the callee's error would only +// hold for an Error() that answers identically every time. The wrapper is +// therefore unconditional: it is the frozen message, not the check, that makes +// this safe by construction. func RunOffCgoStack[T any](fn func() (T, error)) (T, error) { type result struct { val T @@ -63,19 +67,28 @@ type sanitizedError struct { func (e *sanitizedError) Error() string { return e.msg } func (e *sanitizedError) Unwrap() error { return e.err } -func sanitizeForGomobile(err error) error { +func sanitizeForGomobile(err error) (safe error) { if err == nil { return nil } + // Error() is callee-supplied and runs here, on the cgo-callback goroutine, + // outside the recover that guards fn. An interface holding a typed nil + // pointer is non-nil but derefs on the call, and an unrecovered panic in + // the helper whose job is to keep the bridge safe would take the process + // down on the way out. + defer func() { + if r := recover(); r != nil { + slog.Error("panic formatting error for gomobile", "panic", r, "stack", string(debug.Stack())) + safe = &sanitizedError{msg: "unknown error"} + } + }() msg := err.Error() - // The overwhelmingly common case: nothing to fix, so hand back the error - // itself rather than a copy that merely reads the same. - if msg != "" && utf8.ValidString(msg) { - return err - } if !utf8.ValidString(msg) { msg = strings.ToValidUTF8(msg, "?") } + // Not for the bridge's sake — go_seq_to_objc_string returns @"" for an + // empty message and never reaches initWithBytesNoCopy. An error whose + // text is blank is just useless to whoever reads the crash report. if msg == "" { msg = "unknown error" } diff --git a/lantern-core/utils/gostack_test.go b/lantern-core/utils/gostack_test.go index fbc006623c..08df61b570 100644 --- a/lantern-core/utils/gostack_test.go +++ b/lantern-core/utils/gostack_test.go @@ -66,6 +66,49 @@ func TestRunOffCgoStackSanitizesUnsafeMessages(t *testing.T) { } } +// mutatingError answers differently on each call, the way an error formatting a +// buffer another goroutine is still appending to would. The bridge calls +// Error() itself when it builds NSLocalizedDescriptionKey, so validating a +// message and handing back the callee's error would let the second answer — +// the one that actually gets marshalled — be anything at all. +type mutatingError struct{ calls int } + +func (e *mutatingError) Error() string { + e.calls++ + if e.calls == 1 { + return "fine" + } + return "bad\xffbytes" +} + +func TestRunOffCgoStackFreezesTheMessage(t *testing.T) { + in := &mutatingError{} + _, got := RunOffCgoStack(func() (struct{}, error) { return struct{}{}, in }) + first := got.Error() + if first != "fine" { + t.Fatalf("first read: got %q, want %q", first, "fine") + } + if second := got.Error(); second != first { + t.Errorf("message not frozen: bridge would marshal %q after seeing %q", second, first) + } +} + +// A nil *typedNilError inside a non-nil error interface derefs on Error(). +type typedNilError struct{ msg string } + +func (e *typedNilError) Error() string { return e.msg } + +func TestRunOffCgoStackSurvivesTypedNilError(t *testing.T) { + var typed *typedNilError + _, got := RunOffCgoStack(func() (struct{}, error) { return struct{}{}, typed }) + if got == nil { + t.Fatal("a non-nil error interface must not come back nil") + } + if got.Error() == "" { + t.Error("the bridge needs a non-empty message even for this shape") + } +} + func TestRunOffCgoStackNilErrorStaysNil(t *testing.T) { if _, err := RunOffCgoStack(func() (int, error) { return 7, nil }); err != nil { t.Fatalf("nil error became %v", err) From 66855102ea2ce78a79ee98357b62aedbcf670fc6 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 16 Aug 2026 01:18:56 -0600 Subject: [PATCH 2/2] utils: keep the cause reachable through the panic recovery Review caught that the recovery path built a sanitizedError with no cause, so errors.Is and errors.As stopped at the wrapper exactly where this change claims they keep working. An error whose Error() panics can still be the sentinel a caller is testing for, and dropping it made recovery silently change the answer. The typed-nil test now asserts both traversals rather than only that the call survives, which is what would have caught this. Co-Authored-By: Claude Opus 5 (1M context) --- lantern-core/utils/gostack.go | 6 +++++- lantern-core/utils/gostack_test.go | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lantern-core/utils/gostack.go b/lantern-core/utils/gostack.go index 7564ddc558..1c2328c342 100644 --- a/lantern-core/utils/gostack.go +++ b/lantern-core/utils/gostack.go @@ -79,7 +79,11 @@ func sanitizeForGomobile(err error) (safe error) { defer func() { if r := recover(); r != nil { slog.Error("panic formatting error for gomobile", "panic", r, "stack", string(debug.Stack())) - safe = &sanitizedError{msg: "unknown error"} + // Keep the cause reachable even here. An error whose Error() + // panics can still be the sentinel a caller is testing for, and + // dropping it would make recovery silently change what errors.Is + // answers. + safe = &sanitizedError{msg: "unknown error", err: err} } }() msg := err.Error() diff --git a/lantern-core/utils/gostack_test.go b/lantern-core/utils/gostack_test.go index 08df61b570..b7adbce2ed 100644 --- a/lantern-core/utils/gostack_test.go +++ b/lantern-core/utils/gostack_test.go @@ -107,6 +107,16 @@ func TestRunOffCgoStackSurvivesTypedNilError(t *testing.T) { if got.Error() == "" { t.Error("the bridge needs a non-empty message even for this shape") } + // Recovering from the panic must not quietly cost the caller the cause; + // an error that panics while formatting can still be the one they are + // testing for. + if !errors.Is(got, error(typed)) { + t.Error("the recovered error no longer unwraps to the original") + } + var as *typedNilError + if !errors.As(got, &as) { + t.Error("errors.As cannot reach the original through the recovery path") + } } func TestRunOffCgoStackNilErrorStaysNil(t *testing.T) {