From fcf88788237181b020c55ef65dd379f4592119e6 Mon Sep 17 00:00:00 2001 From: Ilya Torbin Date: Fri, 10 Jul 2026 20:13:06 +0300 Subject: [PATCH 1/2] log/slog: test marshaler output escaping Check that MarshalText and MarshalJSON output is escaped when writing JSON log records. Change-Id: Iacda60699c4167246d8b78ebed6a9fb5e33c1840 Reviewed-on: https://go-review.googlesource.com/c/go/+/799420 Reviewed-by: Sean Liao Auto-Submit: Jonathan Amsterdam Reviewed-by: Jonathan Amsterdam LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Michael Pratt Auto-Submit: Sean Liao --- src/log/slog/handler_test.go | 2 -- src/log/slog/json_handler_test.go | 36 +++++++++++++++++++++++++++++-- src/log/slog/text_handler_test.go | 14 ++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/log/slog/handler_test.go b/src/log/slog/handler_test.go index 04c851d731dc89..6216c54e993e17 100644 --- a/src/log/slog/handler_test.go +++ b/src/log/slog/handler_test.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// TODO: verify that the output of Marshal{Text,JSON} is suitably escaped. - package slog import ( diff --git a/src/log/slog/json_handler_test.go b/src/log/slog/json_handler_test.go index 24b79ef62bb84c..9ea0f1c74311cd 100644 --- a/src/log/slog/json_handler_test.go +++ b/src/log/slog/json_handler_test.go @@ -9,7 +9,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "internal/goexperiment" "io" "log/slog/internal/buffer" @@ -54,6 +53,39 @@ func TestJSONHandler(t *testing.T) { } } +func TestJSONHandlerMarshalerEscaping(t *testing.T) { + var buf bytes.Buffer + h := NewJSONHandler(&buf, nil) + r := NewRecord(testTime, LevelInfo, "m", 0) + r.AddAttrs(Any("m", jsonMarshaler{marshalerASCII})) + if err := h.Handle(t.Context(), r); err != nil { + t.Fatal(err) + } + got := bytes.TrimSuffix(buf.Bytes(), []byte{'\n'}) + if !json.Valid(got) { + t.Fatalf("output is not valid JSON: %q", got) + } + for _, escaped := range []string{`\"`, `\\`, `\t`, `\n`, `\u0000`} { + if !bytes.Contains(got, []byte(escaped)) { + t.Errorf("output %q does not contain JSON escape %q", got, escaped) + } + } + for i := byte(0); i < 0x20; i++ { + if bytes.IndexByte(got, i) >= 0 { + t.Errorf("output %q contains unescaped control character %#x", got, i) + } + } + var record struct { + M []string `json:"m"` + } + if err := json.Unmarshal(got, &record); err != nil { + t.Fatal(err) + } + if len(record.M) != 1 || record.M[0] != marshalerASCII { + t.Errorf("marshaled value = %q, want %q", record.M, []string{marshalerASCII}) + } +} + // for testing json.Marshaler type jsonMarshaler struct { s string @@ -65,7 +97,7 @@ func (j jsonMarshaler) MarshalJSON() ([]byte, error) { if j.s == "" { return nil, errors.New("json: empty string") } - return []byte(fmt.Sprintf(`[%q]`, j.s)), nil + return json.Marshal([]string{j.s}) } type jsonMarshalerError struct { diff --git a/src/log/slog/text_handler_test.go b/src/log/slog/text_handler_test.go index 35abf9a272f38f..26890cf7e197ab 100644 --- a/src/log/slog/text_handler_test.go +++ b/src/log/slog/text_handler_test.go @@ -11,6 +11,7 @@ import ( "fmt" "internal/testenv" "io" + "strconv" "strings" "testing" "time" @@ -18,6 +19,14 @@ import ( var testTime = time.Date(2000, 1, 2, 3, 4, 5, 0, time.UTC) +var marshalerASCII = func() string { + b := make([]byte, 128) + for i := range b { + b[i] = byte(i) + } + return string(b) +}() + func TestTextHandler(t *testing.T) { for _, test := range []struct { name string @@ -49,6 +58,11 @@ func TestTextHandler(t *testing.T) { Any("t", text{"abc"}), `t`, `"text{\"abc\"}"`, }, + { + "TextMarshaler escapes", + Any("t", text{marshalerASCII}), + `t`, strconv.Quote(fmt.Sprintf("text{%q}", marshalerASCII)), + }, { "TextMarshaler error", Any("t", text{""}), From a4d587ad78fede291986c1becf687a2dcdeb42fd Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 14 Jul 2026 11:02:09 -0400 Subject: [PATCH 2/2] std: address ptrtoerror analyzer diagnostics Gopls' new ptrtoerror analyzer (CL 798580) reports types E such that both E and *E implement error and there is no clear intent as to which one is preferred. This CL cleans up nearly all such diagnostics in std by adding explicit assertions such as 'var _ error = E{}'. Two mistaken uses of scanner.Error (sans &) were fixed, and several unnecessary embeddings of error in tests were change to use a plain named field. Change-Id: I1dbf4abd5750e69c34653a1cdd17fa04edfbc6e4 Reviewed-on: https://go-review.googlesource.com/c/go/+/800560 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov Commit-Queue: Alan Donovan Auto-Submit: Alan Donovan --- src/cmd/go/internal/modindex/build_read.go | 2 +- src/crypto/tls/alert.go | 2 ++ src/crypto/tls/tls.go | 2 ++ src/debug/elf/reader.go | 2 +- src/go/build/read.go | 2 +- src/go/internal/gccgoimporter/parser.go | 2 ++ src/go/scanner/errors.go | 2 ++ src/go/scanner/scanner_test.go | 2 +- src/internal/reflectlite/set_test.go | 2 ++ src/internal/runtime/exithook/hooks.go | 4 ---- src/log/slog/json_handler_test.go | 2 ++ src/math/big/float.go | 2 ++ src/net/dial.go | 2 +- src/net/dial_test.go | 2 +- src/net/dnsclient_unix.go | 2 +- src/net/http/h2_error_test.go | 2 ++ src/net/net.go | 2 ++ src/runtime/error.go | 6 ++++++ src/runtime/synctest.go | 2 ++ 19 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/cmd/go/internal/modindex/build_read.go b/src/cmd/go/internal/modindex/build_read.go index 86b908a9feae3d..0eb28e8b98576f 100644 --- a/src/cmd/go/internal/modindex/build_read.go +++ b/src/cmd/go/internal/modindex/build_read.go @@ -328,7 +328,7 @@ func readGoInfo(f io.Reader, info *fileInfo) error { if !isValidImport(path) { // The parser used to return a parse error for invalid import paths, but // no longer does, so check for and create the error here instead. - info.parseErr = scanner.Error{Pos: info.fset.Position(spec.Pos()), Msg: "invalid import path: " + path} + info.parseErr = &scanner.Error{Pos: info.fset.Position(spec.Pos()), Msg: "invalid import path: " + path} info.imports = nil return nil } diff --git a/src/crypto/tls/alert.go b/src/crypto/tls/alert.go index 2301c0673d8387..846c7d670b8c7a 100644 --- a/src/crypto/tls/alert.go +++ b/src/crypto/tls/alert.go @@ -12,6 +12,8 @@ import "strconv" // which wraps AlertError rather than sending a TLS alert. type AlertError uint8 +var _ error = AlertError(0) + func (e AlertError) Error() string { return alert(e).String() } diff --git a/src/crypto/tls/tls.go b/src/crypto/tls/tls.go index 5a710a3b88dbfa..d832470cf5d756 100644 --- a/src/crypto/tls/tls.go +++ b/src/crypto/tls/tls.go @@ -112,6 +112,8 @@ func Listen(network, laddr string, config *Config) (net.Listener, error) { type timeoutError struct{} +var _ error = timeoutError{} + func (timeoutError) Error() string { return "tls: DialWithDialer timed out" } func (timeoutError) Timeout() bool { return true } func (timeoutError) Temporary() bool { return true } diff --git a/src/debug/elf/reader.go b/src/debug/elf/reader.go index eab437318d6d3e..e91ec3a2b174c6 100644 --- a/src/debug/elf/reader.go +++ b/src/debug/elf/reader.go @@ -11,7 +11,7 @@ import ( // errorReader returns error from all operations. type errorReader struct { - error + error error } func (r errorReader) Read(p []byte) (n int, err error) { diff --git a/src/go/build/read.go b/src/go/build/read.go index 737f409d4eb427..1d71d2995cc964 100644 --- a/src/go/build/read.go +++ b/src/go/build/read.go @@ -335,7 +335,7 @@ func readGoInfo(f io.Reader, info *fileInfo) error { if !isValidImport(path) { // The parser used to return a parse error for invalid import paths, but // no longer does, so check for and create the error here instead. - info.parseErr = scanner.Error{Pos: info.fset.Position(spec.Pos()), Msg: "invalid import path: " + path} + info.parseErr = &scanner.Error{Pos: info.fset.Position(spec.Pos()), Msg: "invalid import path: " + path} info.imports = nil return nil } diff --git a/src/go/internal/gccgoimporter/parser.go b/src/go/internal/gccgoimporter/parser.go index 4f3e2bb4465438..c76d22569da696 100644 --- a/src/go/internal/gccgoimporter/parser.go +++ b/src/go/internal/gccgoimporter/parser.go @@ -75,6 +75,8 @@ type importError struct { err error } +var _ error = importError{} + func (e importError) Error() string { return fmt.Sprintf("import error %s (byte offset = %d): %s", e.pos, e.pos.Offset, e.err) } diff --git a/src/go/scanner/errors.go b/src/go/scanner/errors.go index 3230f1339a6eb5..f860c383ecdae6 100644 --- a/src/go/scanner/errors.go +++ b/src/go/scanner/errors.go @@ -20,6 +20,8 @@ type Error struct { Msg string } +var _ error = (*Error)(nil) // *Error (not Error) is the correct type in error.Is tests. + // Error implements the error interface. func (e Error) Error() string { if e.Pos.Filename != "" || e.Pos.IsValid() { diff --git a/src/go/scanner/scanner_test.go b/src/go/scanner/scanner_test.go index 118bcdef0c4d9a..c231827148ffa3 100644 --- a/src/go/scanner/scanner_test.go +++ b/src/go/scanner/scanner_test.go @@ -577,7 +577,7 @@ func testSegments(t *testing.T, segments []segment, filename string) { // verify scan var S Scanner file := fset.AddFile(filename, fset.Base(), len(src)) - S.Init(file, []byte(src), func(pos token.Position, msg string) { t.Error(Error{pos, msg}) }, dontInsertSemis) + S.Init(file, []byte(src), func(pos token.Position, msg string) { t.Error(&Error{pos, msg}) }, dontInsertSemis) for _, s := range segments { p, _, lit := S.Scan() pos := file.Position(p) diff --git a/src/internal/reflectlite/set_test.go b/src/internal/reflectlite/set_test.go index ca7ea9b0bc39d9..ad1b66a219f17b 100644 --- a/src/internal/reflectlite/set_test.go +++ b/src/internal/reflectlite/set_test.go @@ -59,6 +59,8 @@ type mapError map[string]string func (mapError) Error() string { return "mapError" } +// For this test, both mapError and *mapError implement error, +// though in general this leads to ambiguity. var _ error = mapError{} var _ error = new(mapError) diff --git a/src/internal/runtime/exithook/hooks.go b/src/internal/runtime/exithook/hooks.go index 8dcfb9ed3c06c5..9c3099d4f1ef1e 100644 --- a/src/internal/runtime/exithook/hooks.go +++ b/src/internal/runtime/exithook/hooks.go @@ -79,7 +79,3 @@ func Run(code int) { h.F() } } - -type exitError string - -func (e exitError) Error() string { return string(e) } diff --git a/src/log/slog/json_handler_test.go b/src/log/slog/json_handler_test.go index 9ea0f1c74311cd..7e4452f2e7a9cf 100644 --- a/src/log/slog/json_handler_test.go +++ b/src/log/slog/json_handler_test.go @@ -104,6 +104,8 @@ type jsonMarshalerError struct { jsonMarshaler } +var _ error = jsonMarshalerError{} + func (jsonMarshalerError) Error() string { return "oops" } func TestAppendJSONValue(t *testing.T) { diff --git a/src/math/big/float.go b/src/math/big/float.go index b99088a733f5df..a27fe2ad58095c 100644 --- a/src/math/big/float.go +++ b/src/math/big/float.go @@ -78,6 +78,8 @@ type ErrNaN struct { msg string } +var _ error = ErrNaN{} + func (err ErrNaN) Error() string { return err.msg } diff --git a/src/net/dial.go b/src/net/dial.go index 8a1376d40078b2..af856256c080f4 100644 --- a/src/net/dial.go +++ b/src/net/dial.go @@ -666,7 +666,7 @@ func (sd *sysDialer) dialParallel(ctx context.Context, primaries, fallbacks addr type dialResult struct { Conn - error + error error primary bool done bool } diff --git a/src/net/dial_test.go b/src/net/dial_test.go index 7ddf8b9882c0e6..d777450aee7627 100644 --- a/src/net/dial_test.go +++ b/src/net/dial_test.go @@ -526,7 +526,7 @@ func TestDialerLocalAddr(t *testing.T) { type test struct { network, raddr string laddr Addr - error + error error } var tests = []test{ {"tcp4", "127.0.0.1", nil, nil}, diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index 53c98f78740d76..6e749882c25c6a 100644 --- a/src/net/dnsclient_unix.go +++ b/src/net/dnsclient_unix.go @@ -661,7 +661,7 @@ func (r *Resolver) goLookupIPCNAMEOrder(ctx context.Context, network, name strin type result struct { p dnsmessage.Parser server string - error + error error } if conf == nil { diff --git a/src/net/http/h2_error_test.go b/src/net/http/h2_error_test.go index 1389858d79f7e2..9cbb189a781829 100644 --- a/src/net/http/h2_error_test.go +++ b/src/net/http/h2_error_test.go @@ -21,6 +21,8 @@ type externalStreamError struct { Cause error } +var _ error = externalStreamError{} + func (e externalStreamError) Error() string { return fmt.Sprintf("ID %v, code %v", e.StreamID, e.Code) } diff --git a/src/net/net.go b/src/net/net.go index 72f57721550982..d934a375bb5d80 100644 --- a/src/net/net.go +++ b/src/net/net.go @@ -606,6 +606,8 @@ func (e UnknownNetworkError) Temporary() bool { return false } type InvalidAddrError string +var _ error = InvalidAddrError("") + func (e InvalidAddrError) Error() string { return string(e) } func (e InvalidAddrError) Timeout() bool { return false } func (e InvalidAddrError) Temporary() bool { return false } diff --git a/src/runtime/error.go b/src/runtime/error.go index f95b14d7808189..01bf503ba7819e 100644 --- a/src/runtime/error.go +++ b/src/runtime/error.go @@ -103,6 +103,8 @@ func (e errorAddressString) Error() string { return "runtime error: " + e.msg } +var _ error = errorAddressString{} + // Addr returns the memory address where a fault occurred. // The address provided is best-effort. // The veracity of the result may depend on the platform. @@ -123,6 +125,8 @@ func (e plainError) Error() string { return string(e) } +var _ error = plainError("") + // A boundsError represents an indexing or slicing operation gone wrong. type boundsError struct { x int64 @@ -135,6 +139,8 @@ type boundsError struct { code abi.BoundsErrorCode } +var _ error = boundsError{} + // boundsErrorFmts provide error text for various out-of-bounds panics. // Note: if you change these strings, you should adjust the size of the buffer // in boundsError.Error below as well. diff --git a/src/runtime/synctest.go b/src/runtime/synctest.go index 64a98dad073293..a2b7495d914529 100644 --- a/src/runtime/synctest.go +++ b/src/runtime/synctest.go @@ -258,6 +258,8 @@ type synctestDeadlockError struct { bubble *synctestBubble } +var _ error = synctestDeadlockError{} + func (e synctestDeadlockError) Error() string { return e.reason }