-
-
Notifications
You must be signed in to change notification settings - Fork 216
Fix incorrect JSON string escaping on big-endian architectures (s390x) #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| // escape_endianness_test.go | ||
| // | ||
| // Regression tests for the big-endian (e.g. s390x) string-escaping bug, where | ||
| // the SWAR escape scanner in internal/encoder/string.go mislocated the first | ||
| // byte needing escaping and emitted the first `"` of a value unescaped, | ||
| // producing invalid JSON | ||
|
|
||
| package json_test | ||
|
|
||
| import ( | ||
| stdjson "encoding/json" | ||
| "reflect" | ||
| "strings" | ||
| "testing" | ||
| "unicode/utf8" | ||
|
|
||
| gojson "github.com/goccy/go-json" | ||
| ) | ||
|
|
||
| // TestMarshalEmbeddedJSONStrings mirrors annotation values | ||
| // that are themselves serialized JSON objects/arrays. The first interior quote | ||
| // of each value must be escaped; on s390x it was not. | ||
| func TestMarshalEmbeddedJSONStrings(t *testing.T) { | ||
| m := map[string]string{ | ||
| "Annotations": `{"io.kubernetes.container.hash":"a539e690","restartCount":"0"}`, | ||
| "Volumes": `[{"container_path":"/host","host_path":"/","readonly":false}]`, | ||
| "leadingQuote": `"x`, // quote as the very first byte: the s390x-broken case | ||
| "plain": "no escaping needed", | ||
| } | ||
|
|
||
| got, err := gojson.Marshal(m) | ||
| if err != nil { | ||
| t.Fatalf("Marshal: %v", err) | ||
| } | ||
| if !stdjson.Valid(got) { | ||
| t.Fatalf("produced invalid JSON: %s", got) | ||
| } | ||
|
|
||
| var round map[string]string | ||
| if err := stdjson.Unmarshal(got, &round); err != nil { | ||
| t.Fatalf("output not parseable by encoding/json: %v\n%s", err, got) | ||
| } | ||
| if !reflect.DeepEqual(round, m) { | ||
| t.Fatalf("round-trip mismatch:\n got %#v\nwant %#v", round, m) | ||
| } | ||
| } | ||
|
|
||
| // TestMarshalStringEscapingMatchesStdlib checks a fixed set of strings that | ||
| // straddle 8-byte SWAR chunk boundaries, where the byte-offset computation is | ||
| // endianness-sensitive. | ||
| // | ||
| // Comparison is semantic (round-trip decode) rather than byte-equal because | ||
| // go-json intentionally encodes \b and \f as \u0008/\u000c while encoding/json | ||
| // uses the named escapes \b/\f. Both are valid per RFC 8259 §7 and decode to | ||
| // the same value. | ||
| func TestMarshalStringEscapingMatchesStdlib(t *testing.T) { | ||
| cases := []string{ | ||
| ``, | ||
| `"`, | ||
| `\`, | ||
| `"abcdefg`, // quote at byte 0 of chunk 0 | ||
| `abcdefg"`, // quote at byte 7 of chunk 0 | ||
| `abcdefgh"ij`, // quote at byte 0 of chunk 1 | ||
| `abcdefghijklmno"pqr`, // quote at byte 7 of chunk 1 | ||
| "\x00\x1f", // control characters (includes \b 0x08, \f 0x0c) | ||
| `<&>`, // HTML-significant characters | ||
| strings.Repeat(`"`, 9), | ||
| "héllo wörld", // multibyte UTF-8 | ||
| "\xe4\xb8\xad", // CJK byte sequence (中) | ||
| "tab\tnewline\n", // common escapes | ||
| } | ||
| for _, s := range cases { | ||
| got, err := gojson.Marshal(s) | ||
| if err != nil { | ||
| t.Fatalf("Marshal(%q): %v", s, err) | ||
| } | ||
| if !stdjson.Valid(got) { | ||
| t.Errorf("Marshal(%q) produced invalid JSON: %s", s, got) | ||
| continue | ||
| } | ||
| assertSemanticEqual(t, s, got) | ||
| } | ||
| } | ||
|
|
||
| // FuzzMarshalStringMatchesStdlib asserts that goccy/go-json produces valid JSON | ||
| // that round-trips identically to encoding/json for all valid UTF-8 strings. | ||
| // This is the most effective check for the endianness bug: run it on s390x | ||
| // (see file header). | ||
| // | ||
| // Note: byte-for-byte equality with encoding/json is intentionally NOT | ||
| // required. go-json encodes \b (0x08) as \u0008 and \f (0x0c) as \u000c, | ||
| // while encoding/json uses the named escapes \b/\f. Both are valid per | ||
| // RFC 8259 §7 and decode to identical values. The contract here is semantic | ||
| // equivalence, not identical wire bytes. | ||
| func FuzzMarshalStringMatchesStdlib(f *testing.F) { | ||
| seeds := []string{ | ||
| ``, `"`, `\`, `{"a":"b"}`, "\x00\x1f", `<&>`, | ||
| strings.Repeat(`"`, 9), `abcdefgh"ij`, "héllo", "\xe4\xb8\xad", | ||
| "\b", "\f", "00\b", "00\f", // explicit seeds for the known \b/\f divergence | ||
| } | ||
| for _, s := range seeds { | ||
| f.Add(s) | ||
| } | ||
| f.Fuzz(func(t *testing.T, s string) { | ||
| if !utf8.ValidString(s) { | ||
| t.Skip() // invalid UTF-8: not part of this contract | ||
| } | ||
| got, err := gojson.Marshal(s) | ||
| if err != nil { | ||
| t.Fatalf("Marshal(%q) returned unexpected error: %v", s, err) | ||
| } | ||
| if !stdjson.Valid(got) { | ||
| t.Fatalf("Marshal(%q) produced invalid JSON: %s", s, got) | ||
| } | ||
| assertSemanticEqual(t, s, got) | ||
| }) | ||
| } | ||
|
|
||
| // assertSemanticEqual decodes got (produced by go-json) and the encoding/json | ||
| // output for the same input s, then compares the decoded values with | ||
| // reflect.DeepEqual. This correctly handles cases where go-json and | ||
| // encoding/json choose different but equally valid escape sequences. | ||
| func assertSemanticEqual(t *testing.T, s string, got []byte) { | ||
| t.Helper() | ||
| want, _ := stdjson.Marshal(s) | ||
|
|
||
| var gotVal, wantVal string | ||
| if err := stdjson.Unmarshal(got, &gotVal); err != nil { | ||
| t.Fatalf("could not decode go-json output for %q: %v — raw: %s", s, err, got) | ||
| } | ||
| if err := stdjson.Unmarshal(want, &wantVal); err != nil { | ||
| t.Fatalf("could not decode stdlib output for %q: %v — raw: %s", s, err, want) | ||
| } | ||
| if !reflect.DeepEqual(gotVal, wantVal) { | ||
| t.Fatalf("semantic mismatch for %q:\n got=%s (decoded: %q)\nwant=%s (decoded: %q)", | ||
| s, got, gotVal, want, wantVal) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| package encoder | ||
|
|
||
| import ( | ||
| "encoding/binary" | ||
| "math/bits" | ||
| "reflect" | ||
| "unsafe" | ||
|
|
@@ -35,10 +36,38 @@ const ( | |
| msb = 0x8080808080808080 | ||
| ) | ||
|
|
||
| // isBigEndian reports whether the host CPU is big-endian. | ||
| // | ||
| // encoding/binary.NativeEndian interprets memory using the host's native byte | ||
| // order. By reading the two-byte sequence [0x01, 0x00] as a uint16: | ||
| // - big-endian host: first byte is the MSB → result is 0x0100 → true | ||
| // - little-endian host: first byte is the LSB → result is 0x0001 → false | ||
| var isBigEndian = binary.NativeEndian.Uint16([]byte{0x01, 0x00}) == 0x0100 | ||
|
|
||
| var hex = "0123456789abcdef" | ||
|
|
||
| //nolint:govet | ||
| func stringToUint64Slice(s string) []uint64 { | ||
| if isBigEndian { | ||
| // On big-endian hosts (e.g. s390x), the native unsafe reinterpretation | ||
| // used by the little-endian path below would place byte 0 of each chunk | ||
| // in the most-significant lane of the uint64 word. The SWAR escape | ||
| // scanners use bits.TrailingZeros64(mask&msb)/8 to locate the first byte | ||
| // needing escaping, which assumes byte 0 is in the least-significant lane. | ||
| // To satisfy that invariant, read each 8-byte chunk explicitly in | ||
| // little-endian order via binary.LittleEndian.Uint64. | ||
| n := len(s) / 8 | ||
| buf := make([]uint64, n) | ||
| data := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{ | ||
| Data: ((*reflect.StringHeader)(unsafe.Pointer(&s))).Data, | ||
| Len: len(s), | ||
| Cap: len(s), | ||
| })) | ||
| for i := 0; i < n; i++ { | ||
| buf[i] = binary.LittleEndian.Uint64(data[i*8 : i*8+8]) | ||
| } | ||
| return buf | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashokpariya0.. I had a suggestion here.. You can add ""encoding/binary" package and use the in-built function such as "binary.LittleEndian.PutUint64()" or "binary.BigEndian.PutUint64()" which ever is appropriate..
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated as suggested, change |
||
| return *(*[]uint64)(unsafe.Pointer(&reflect.SliceHeader{ | ||
| Data: ((*reflect.StringHeader)(unsafe.Pointer(&s))).Data, | ||
| Len: len(s) / 8, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| // internal/encoder/string_escape_index_test.go | ||
| // | ||
| // goccy/go-json has no standalone escapeIndex function; the SWAR escape scan is | ||
| // inlined into the append*String functions in string.go. These tests drive | ||
| // those functions directly, which exercises the scan together with the | ||
| // stringToUint64Slice loader — specifically the s390x branch selected by the | ||
| // bigEndian compile-time constant in string.go. | ||
| // | ||
| // The boundary cases (quote at byte 0 / 7 / 8) are exactly where the s390x | ||
| // byte-offset computation used to be wrong, emitting the first `"` unescaped | ||
| // Output is compared against encoding/json, which goccy | ||
| // aims to match byte-for-byte. | ||
|
|
||
| package encoder | ||
|
|
||
| import ( | ||
| "bytes" | ||
| stdjson "encoding/json" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| // stdHTMLEscaped mirrors appendHTMLString: encoding/json HTML-escapes by default. | ||
| func stdHTMLEscaped(s string) []byte { | ||
| b, err := stdjson.Marshal(s) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return b | ||
| } | ||
|
|
||
| // stdNoHTMLEscaped mirrors appendString: HTML escaping disabled. | ||
| func stdNoHTMLEscaped(s string) []byte { | ||
| var buf bytes.Buffer | ||
| enc := stdjson.NewEncoder(&buf) | ||
| enc.SetEscapeHTML(false) | ||
| if err := enc.Encode(s); err != nil { | ||
| panic(err) | ||
| } | ||
| return bytes.TrimRight(buf.Bytes(), "\n") // Encode appends a trailing newline | ||
| } | ||
|
|
||
| func TestAppendStringEscaping(t *testing.T) { | ||
| cases := []string{ | ||
| `"abcdefg`, // quote at byte 0 of chunk 0 (the s390x bug) | ||
| `abcdefg"`, // quote at byte 7 of chunk 0 | ||
| `abcdefgh"ijk`, // quote at byte 0 of chunk 1 | ||
| `{"hash":"a539e690"}`, // embedded-JSON value, like cri-o annotations | ||
| `[{"container_path":"/host"}]`, // embedded-JSON array value | ||
| "plain ascii, nothing special", | ||
| "\x00\x01\x1f control chars", | ||
| "tab\tnewline\ncr\r", | ||
| `back\slash and "quotes"`, | ||
| "unicode héllo wörld 中文 🚀", | ||
| strings.Repeat(`"`, 9), | ||
| "", | ||
| } | ||
|
|
||
| for _, s := range cases { | ||
| if got, want := appendHTMLString(nil, s), stdHTMLEscaped(s); !bytes.Equal(got, want) { | ||
| t.Errorf("appendHTMLString(%q):\n got = %s\n want = %s", s, got, want) | ||
| } | ||
| if got, want := appendString(nil, s), stdNoHTMLEscaped(s); !bytes.Equal(got, want) { | ||
| t.Errorf("appendString(%q):\n got = %s\n want = %s", s, got, want) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashokpariya0.. Please note that as part of "runtime" we already have the architecture check done and information is available.. Kindly make use of the below check.. Sorry, I didnt observe this earlier.. Thanks..
if runtime.GOARCH == "s390x" {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime.GOARCH is removed, using binary.NativeEndian to cover all big-endian architectures.