Fix fromHex endianness on big-endian architectures (s390x)#48
Conversation
The fromHex function reverses decoded hex bytes into little-endian order, then calls FromBytes() which uses an unsafe pointer cast to reinterpret []byte as []uint64. This cast uses native byte order, producing incorrect results on big-endian systems (e.g. s390x). Replace the unsafe FromBytes() call with explicit binary.LittleEndian.Uint64() decoding so the hex-to-bitmap conversion is correct on all architectures. Fixes kelindar#47
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the fromHex function in codec.go to use explicit little-endian decoding, ensuring compatibility with big-endian architectures like s390x. The reviewer pointed out that the FromBytes function still contains platform-dependent code using unsafe.Pointer and recommended either making it platform-independent or auditing its other usages to prevent similar bugs in other parts of the package.
| // Use explicit little-endian decoding instead of unsafe pointer cast | ||
| // to produce correct results on big-endian architectures (e.g. s390x). | ||
| out := make(Bitmap, len(b)/8) | ||
| for i := range out { | ||
| out[i] = binary.LittleEndian.Uint64(b[i*8 : (i+1)*8]) | ||
| } |
There was a problem hiding this comment.
Replacing the FromBytes call with explicit little-endian decoding correctly addresses the endianness issue for the fromHex function. However, as noted in the pull request description, the FromBytes function itself uses an unsafe.Pointer cast that is sensitive to host byte order. If FromBytes is still used elsewhere in the package (for example, in binary deserialization logic), those call sites will remain broken on big-endian architectures like s390x. It is recommended to either update FromBytes to be platform-independent or audit all other usages to ensure they are not intended for portable data formats.
1 participant
Fixes #47
Hello, first from all, thanks for this project. I was packaging this for debian, and found
the issue in CI that was reported on #47
This is a little patch.
Summary:
The
fromHexfunction reverses decoded hex bytes into little-endianorder, then calls
FromBytes()which uses anunsafe.Pointercast toreinterpret
[]byteas[]uint64. This cast interprets bytes in nativebyte order, so on big-endian systems (e.g. s390x) the uint64 values come
out byte-swapped.
For example, hex string
"FFA001"produces:0xFFA001(correct)0x01A0FF0000000000(wrong)The fix replaces the
FromBytes()call with explicitbinary.LittleEndian.Uint64()decoding, which produces correct resultsregardless of the host byte order. All existing tests continue to pass
on little-endian, and the two failing tests (TestJSON, TestFromHex)
are fixed on big-endian.
For the origin where we found this bug in Debian. CI: where autopkgtest
runs on s390x among other architectures:
https://ci.debian.net/packages/g/golang-github-kelindar-bitmap/testing/s390x/71172418/