json: fix escapeIndex() on big-endian (s390x) - #171
Open
dmkif wants to merge 1 commit into
Open
Conversation
escapeIndex read 8-byte chunks via an unsafe native-endian cast, so bits.TrailingZeros64(...)/8 returned the mirrored byte index on big-endian (s390x). Characters needing escaping were skipped and written raw, producing invalid JSON. Use binary.LittleEndian.Uint64 like parse.go already does. Adds a regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #170 (full story there — chasing a Portainer bug on s390x/zCX, containers with OCI labels not listing).
Putting up a PR since I already had the change sitting locally, but genuinely not sure if this is the "right" fix from a project-conventions standpoint — happy to rework it if you'd rather do it differently. I'm still fairly new to Go so go easy on me if I got a convention wrong somewhere.
What I changed
escapeIndex()was casting 8-byte chunks of the string into auint64viaunsafe.Pointer(thestringToUint64helper), which reads native-endian. On big-endian (s390x) that's the wrong byte order for howbits.TrailingZeros64(...)/8figures out the byte index afterwards — it returns the mirrored position (7-k instead of k) within the chunk, so it can point at the wrong byte and skip a character that should've been escaped.I swapped that for
binary.LittleEndian.Uint64on each 8-byte block (same asparse.goalready does elsewhere in this repo), and added the block offsetito theTrailingZeros64(...)/8result so the index comes out right no matter the host endianness.Since
stringToUint64isn't used anywhere else after that, I removed it and the now-unusedunsafeimport fromjson/string.go. LeftsliceHeaderalone since it's still used injson/codec.go.Testing
Added
TestEscapeIndexBigEndiantojson/string_test.go:escapeIndexagainst a plain byte-by-byte reference scanner for a handful of strings with the special character at different positions (including right around the 8-byte chunk boundary, since that's where things went wrong for me)Marshaland stdlibencoding/json'sUnmarshalto check the actual JSON output is valid and comes back as the same string, similar to the reproducer in escapeIndex() breaks string escaping on big-endian (s390x) #170On amd64:
go build ./...— finego vet ./json— some pre-existing warnings in other test files, unrelated to this change, didn't touch thosego test ./json -run TestEscapeIndexBigEndian— passesI don't have a Go toolchain on the s390x box itself (zCX/Tumbleweed, not really meant for dev work), so I built and ran the test on a big-endian setup I have access to elsewhere — it failed against the old code and passes with this change.
Only touched
json/string.goandjson/string_test.go, didn't touchgo.mod/go.sum.