Skip to content

utils: Read SipHash words byte by byte - #47

Open
jasonsopko wants to merge 1 commit into
CONVOYMining:masterfrom
jasonsopko:convoy-siphash-align
Open

jasonsopko wants to merge 1 commit into
CONVOYMining:masterfrom
jasonsopko:convoy-siphash-align

Conversation

@jasonsopko

Copy link
Copy Markdown

What

datum_siphash and datum_siphash_mod8 read the key and the message words with upk_u64le instead of through a uint64_t cast.

Why

The short txn list reply (0x90) hashes hash_bin, which sits at offset 178 of T_DATUM_TEMPLATE_TXN, so every word load there is misaligned. x86 tolerates it, but it is undefined behavior, and every Linux and FreeBSD CI build has -fsanitize=undefined -fno-sanitize-recover=all. No test sends that request today, so CI is green; the test I suggested on #46 does, and fails under UBSan without this.

How I tested

  • The new test checks the published SipHash-2-4 vectors (lengths 0, 8 and 15) and a 32-byte case with the key and the message misaligned. On master it passes in a plain build and fails under UBSan at datum_utils.c:694, the key load. With this change it passes in the gcc, clang, Umbrel, no-API and ASan+UBSan configs with -Wall -Werror.
  • 2,000 random keys and messages against an independent SipHash-2-4 in Python: master matches at aligned offsets, this change at every offset from 0 to 7, for datum_siphash and, on the 1,116 messages whose length is a multiple of 8, datum_siphash_mod8.
  • Release build, 16,383 32-byte hashes at the hash_bin offset, best of 200, three runs each: 340.0 to 340.7 us with this change and 339.9 to 341.9 us on master, with the same checksum.

Risk and rollback

Output on little-endian hosts is unchanged, and every CI target is little-endian. A big-endian host now reads the words in SipHash's byte order rather than its own. One commit to revert.

Scripts and logs: https://gist.github.com/jasonsopko/7df5dc75053fc4abb65cb7b927c816a8

datum_siphash and datum_siphash_mod8 load the key and each message word
through a uint64_t cast. The short txn list hashes hash_bin, which sits
at offset 178 of T_DATUM_TEMPLATE_TXN, so every word load there is
misaligned. x86 tolerates it, but it is undefined behavior, and
UBSan stops on it.

Read the key and the words with upk_u64le, and build the tail word the
same way. Output on little-endian hosts is unchanged.

The new test checks the published SipHash-2-4 vectors with the key and
message misaligned. On master it fails under UBSan at the key load.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused decoding changes preserve SipHash semantics, and regression tests cover misaligned inputs with no unresolved findings.

No open issues 🎉

What changed in this PR

Makes both SipHash implementations safe for unaligned inputs while preserving little-endian results.

Changes:

  • Replaces cast-based word loads with upk_u64le.
  • Assembles trailing bytes explicitly in little-endian order.
  • Adds reference-vector tests with misaligned keys and messages.
File Description
src/​datum_utils.c Uses alignment-safe, little-endian decoding in both SipHash implementations.
src/​datum_utils_tests.c Adds and registers misaligned-input regression tests.

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants