Fix incorrect JSON string escaping on big-endian architectures (s390x) - #584
Fix incorrect JSON string escaping on big-endian architectures (s390x)#584ashokpariya0 wants to merge 1 commit into
Conversation
a51fbb3 to
619854c
Compare
Vishwanatha-HD
left a comment
Vishwanatha-HD
left a comment
There was a problem hiding this comment.
Hi @ashokpariya0..
Thanks for all your code changes. I would suggest you to take care of the review comment. Thanks..
| uint64(s[j+4])<<32 | uint64(s[j+5])<<40 | uint64(s[j+6])<<48 | uint64(s[j+7])<<56 | ||
| } | ||
| return buf | ||
| } |
There was a problem hiding this comment.
@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..
These in-built functions are tested already, it will be easy to use and it improves the code readability as well.. Thanks..
Fixes incorrect JSON string escaping on big-endian architectures (s390x). The first `"` of a value was emitted unescaped, producing invalid JSON. On big-endian (s390x), stringToUint64Slice loaded each 8-byte word with a native-endian unsafe cast, so byte 0 landed in the most-significant lane. The SWAR escape scanners use bits.TrailingZeros64(mask&msb)/8, which expects byte 0 in the least-significant lane, so they mislocated the first byte to escape and emitted the first `"` unescaped, producing invalid JSON. Verified on native s390x; also fixes pre-existing suite failures (TestHTMLEscape, TestCompactBig, TestIndentBig, TestEncoderSetEscapeHTML, TestUnmarshalMarshal, TestUnmarshalRescanLiteralMangledUnquote). Adds regression, differential, and fuzz tests. Signed-off-by: Ashok Pariya <ashok.pariya@ibm.com>
619854c to
33fed3f
Compare
|
@goccy could you please review this PR? |
Vishwanatha-HD
left a comment
Vishwanatha-HD
left a comment
There was a problem hiding this comment.
Please take care of the review comment.. Thanks
| // - 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 | ||
|
|
There was a problem hiding this comment.
@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.
runtime.GOARCH is removed, using binary.NativeEndian to cover all big-endian architectures.
|
@goccy |
2 participants
Why we need this PR??
Fixes: #466
This PR fixes incorrect JSON string escaping on big-endian architectures (s390x).
The first " of a value was emitted unescaped, producing invalid JSON.
stringToUint64Slice reinterpreted a string's bytes as []uint64 via an
unsafe native-endian load. The SWAR escape scanners in string.go then
locate the first byte needing escaping with
bits.TrailingZeros64(mask&msb)/8, which assumes byte 0 sits in the
least-significant lane of the word. That holds on little-endian, but on
big-endian (s390x) byte 0 lands in the most-significant lane, so the
scanner mislocated the first escapable byte and emitted the first
"ofa value unescaped, producing invalid JSON.
Guard the loader with a compile-time constant (derived from
runtime.GOARCH, so the dead branch is eliminated and the little-endian
path keeps its zero-copy codegen). On big-endian, assemble each word
from its bytes with explicit little-endian shifts so the downstream
mask/TrailingZeros math stays valid. The append*String functions are
unchanged.
Verified on native s390x: this also fixes pre-existing failures in the
project's own suite (TestHTMLEscape, TestCompactBig, TestIndentBig,
TestEncoderSetEscapeHTML, TestUnmarshalMarshal,
TestUnmarshalRescanLiteralMangledUnquote). Adds regression,
differential, and fuzz tests.
Testing results: