diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 154c30c..3c794c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,21 +82,26 @@ jobs: with: platforms: s390x - - name: Run endian tests on s390x + - name: Run encoding tests on s390x run: | docker run --rm --platform linux/s390x \ -v "${{ github.workspace }}:/work" \ -w /work \ - python:3.13-bookworm \ + golang:1.25-bookworm \ bash -euxo pipefail -c ' apt-get update - apt-get install -y --no-install-recommends gcc make + apt-get install -y --no-install-recommends gcc make python3 python3-venv + python3 -m venv /tmp/bitproto-venv + . /tmp/bitproto-venv/bin/activate python -m pip install -r compiler/requirements.txt -r tests/requirements_tests.txt printf "%s\n" \ "#!/usr/bin/env sh" \ - "PYTHONPATH=/work/compiler exec python -m bitproto._main \"\$@\"" \ + "PYTHONPATH=/work/compiler exec /tmp/bitproto-venv/bin/python -m bitproto._main \"\$@\"" \ >/usr/local/bin/bitproto chmod +x /usr/local/bin/bitproto + export GOFLAGS=-buildvcs=false PYTHONPATH=/work/compiler:/work/lib/py python -c "import sys; assert sys.byteorder == \"big\", sys.byteorder" - PYTHONPATH=/work/compiler:/work/lib/py python -m pytest tests/test_encoding/test_endian.py -v + PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c pytest tests/test_encoding -v -s -x + PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c BP_TEST_CC_OPTIMIZATION=-O2 pytest tests/test_encoding -v -s -x + PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c BP_TEST_OPTIMIZATION_ARG=-O pytest tests/test_encoding -v -s -x ' diff --git a/changes.rst b/changes.rst index a05586a..273c159 100644 --- a/changes.rst +++ b/changes.rst @@ -1,5 +1,14 @@ .. currentmodule:: bitproto +Version 1.3.2 +------------- + +.. _version-1.3.2: + +- Bugfix: Fix sign extension for arbitrary-width signed integers such as + ``int62`` in the standard-mode C runtime. This avoids undefined shifts and + fixes decoding these fields on big-endian hosts. + Version 1.3.1 ------------- diff --git a/compiler/bitproto/__init__.py b/compiler/bitproto/__init__.py index 581b4fb..382d30f 100644 --- a/compiler/bitproto/__init__.py +++ b/compiler/bitproto/__init__.py @@ -8,5 +8,5 @@ """ -__version__ = "1.3.1" +__version__ = "1.3.2" __description__ = "bit level data interchange format." diff --git a/compiler/pyproject.toml b/compiler/pyproject.toml index a2187a7..7248500 100644 --- a/compiler/pyproject.toml +++ b/compiler/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "bitproto" -version = "1.3.1" +version = "1.3.2" description = "bit level data interchange format." readme = "README.md" authors = [ diff --git a/lib/c/bitproto.c b/lib/c/bitproto.c index d254434..9d8f0bf 100644 --- a/lib/c/bitproto.c +++ b/lib/c/bitproto.c @@ -13,12 +13,11 @@ // Clang. __big_endian__ covers legacy TI ARM CGT (armcl). __BIG_ENDIAN__ // covers some other toolchains. // __LITTLE_ENDIAN__ == 0 covers IAR (which always defines it). -#if !defined(BP_BIG_ENDIAN) && ( \ - (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)) || \ - defined(__ARM_BIG_ENDIAN) || \ - defined(__big_endian__) || \ - defined(__BIG_ENDIAN__) || \ - (defined(__LITTLE_ENDIAN__) && (__LITTLE_ENDIAN__ == 0))) +#if !defined(BP_BIG_ENDIAN) && \ + ((defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)) || \ + defined(__ARM_BIG_ENDIAN) || defined(__big_endian__) || \ + defined(__BIG_ENDIAN__) || \ + (defined(__LITTLE_ENDIAN__) && (__LITTLE_ENDIAN__ == 0))) #define BP_BIG_ENDIAN 1 #endif @@ -268,6 +267,7 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di, // Number of bits to process during batch copy. int bits = n + si; + bool copied = false; #ifndef BP_BIG_ENDIAN // These fast paths load multiple bytes through a native-endian @@ -279,28 +279,33 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di, // This way, performance faster x2 than bits copy approach. ((uint32_t *)dst)[0] = ((uint32_t *)(src))[0] >> si; c = 32 - si; + copied = true; } else if (bits >= 16) { // Copy as an uint16 integer. ((uint16_t *)dst)[0] = ((uint16_t *)(src))[0] >> si; c = 16 - si; - } else + copied = true; + } #endif - if (bits >= 8) { - // Copy as an unsigned char. - dst[0] = (src[0] >> si) & 0xff; - c = 8 - si; - } else { - // When bits < 8 and di == 0 - // Copy partial bits inside a byte. - // For the original statement: - // c = BpMinTriple(8 - di, 8 - si, n); - // since di is 0 and bits <8, then 8-di is 8 - // and n <8 , the 8-di won't be the smallest, we - // just pick function BpMin over BpMinTriple for the little - // little performance improvement. - c = BpMin(8 - si, n); - // Also, when di is 0, special case of next case. - dst[0] |= ((src[0] >> si) & ~(0xff << c)); + + if (!copied) { + if (bits >= 8) { + // Copy as an unsigned char. + dst[0] = (src[0] >> si) & 0xff; + c = 8 - si; + } else { + // When bits < 8 and di == 0 + // Copy partial bits inside a byte. + // For the original statement: + // c = BpMinTriple(8 - di, 8 - si, n); + // since di is 0 and bits <8, then 8-di is 8 + // and n <8 , the 8-di won't be the smallest, we + // just pick function BpMin over BpMinTriple for the little + // little performance improvement. + c = BpMin(8 - si, n); + // Also, when di is 0, special case of next case. + dst[0] |= ((src[0] >> si) & ~(0xff << c)); + } } } else { // When di != 0, we have to copy partial bits inside a @@ -320,10 +325,10 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di, // ~(0xff << di << c) Gives a mask to clear higher not-need bits // all to 0, e.g. 00001111 on di=4, c=4; Finally: dst |= src to // copy bits. - // The ch is the first byte at pointer src. If this byte is Zero, - // then there's no need to copy anything, just count c. The - // benchmark on stm32 seems performance is improved by 2us by adding - // an if statement that skip the Zero byte. + // The ch is the first byte at pointer src. If this byte is + // Zero, then there's no need to copy anything, just count c. + // The benchmark on stm32 seems performance is improved by 2us + // by adding an if statement that skip the Zero byte. unsigned char ch = src[0]; if (ch) dst[0] |= ((ch >> si << di) & ~(0xff << di << c)); } @@ -337,13 +342,14 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di, // BpEndecodeBaseType process given base type at given data. // This function guarantees to work geven a nbits > 64 is passed in (the -// batch-array path used on little-endian only; on big-endian arrays are processed -// element by element so nbits here is always a single base type's <= 64 bits). +// batch-array path used on little-endian only; on big-endian arrays are +// processed element by element so nbits here is always a single base type's +// <= 64 bits). void BpEndecodeBaseType(int nbits, struct BpProcessorContext *ctx, void *data) { #ifdef BP_BIG_ENDIAN // Stage the field through a little-endian byte view so the wire stays - // little-endian regardless of host byte order. A single base integer is at - // most 64 bits, so 8 bytes of staging is always enough. + // little-endian regardless of host byte order. A single base integer is + // at most 64 bits, so 8 bytes of staging is always enough. int size = BpBaseTypeStorageSize(nbits); unsigned char le[8] = {0}; unsigned char *p = (unsigned char *)data; @@ -368,11 +374,11 @@ void BpEndecodeBaseType(int nbits, struct BpProcessorContext *ctx, void *data) { } // BpHandleIntSignAfterEndecode processes signed integer at given data after -// this integer is endecode. The most left bit (Nth bit for int{N}) of a signed -// integer indicates the sign. For example 00000101 is a negative integer for a -// int3, but a positive integer for a int4. Where nbits is the number of bits -// for this bitproto signed integer, for int{N}, its the N, the argument size is -// the number of bytes in C language. +// this integer is endecode. The most left bit (Nth bit for int{N}) of a +// signed integer indicates the sign. For example 00000101 is a negative +// integer for a int3, but a positive integer for a int4. Where nbits is the +// number of bits for this bitproto signed integer, for int{N}, its the N, +// the argument size is the number of bytes in C language. void BpHandleIntSignAfterEndecode(int size, int nbits, struct BpProcessorContext *ctx, void *data) { // Signed integer's sign bit processing is only about decoding. @@ -387,34 +393,33 @@ void BpHandleIntSignAfterEndecode(int size, int nbits, switch (n) { // Suppose pointer data points to a int24 value V: - // 1. Check its sign: (V >> (24-1)) & 1 + // 1. Check its sign: V & (1 << (24-1)) // 2. If V is negative: // Make a mask that keeps right 24bits be 0, left 8bits be 1: - // mask = ~(1 << 24 - 1) + // mask = ~((1 << 24) - 1) // Recover the original integer is: V | mask. // - // Why not use V << 8 >> 8 solution here? This depends on arithmetic - // shifting. Which propagates the sign bit on the left vacant bits - // when doing a right shift. But C standard points that this - // behavior is implementation-defined. + // Use unsigned integers to avoid implementation-defined right + // shifts on negative signed values, and to avoid undefined behavior + // such as shifting a plain int by 32 or more bits for int62. case 8: // int8_t - if (((*(int8_t *)data) >> (nbits - 1)) & 1) { - *(int8_t *)data |= ~((1 << nbits) - 1); + if ((*(uint8_t *)data) & ((uint8_t)1 << (nbits - 1))) { + *(uint8_t *)data |= (uint8_t)(~(((uint8_t)1 << nbits) - 1)); } break; case 16: // int16_t - if (((*(int16_t *)data) >> (nbits - 1)) & 1) { - *(int16_t *)data |= ~((1 << nbits) - 1); + if ((*(uint16_t *)data) & ((uint16_t)1 << (nbits - 1))) { + *(uint16_t *)data |= (uint16_t)(~(((uint16_t)1 << nbits) - 1)); } break; case 32: // int32_t - if (((*(int32_t *)data) >> (nbits - 1)) & 1) { - *(int32_t *)data |= ~((1 << nbits) - 1); + if ((*(uint32_t *)data) & ((uint32_t)1 << (nbits - 1))) { + *(uint32_t *)data |= (uint32_t)(~(((uint32_t)1 << nbits) - 1)); } break; case 64: // int64_t - if (((*(int64_t *)data) >> (nbits - 1)) & 1) { - *(int64_t *)data |= ~((1 << nbits) - 1); + if ((*(uint64_t *)data) & ((uint64_t)1 << (nbits - 1))) { + *(uint64_t *)data |= (uint64_t)(~(((uint64_t)1 << nbits) - 1)); } break; } diff --git a/tests/test_encoding/test_encoding.py b/tests/test_encoding/test_encoding.py index 7f0d087..73a6525 100644 --- a/tests/test_encoding/test_encoding.py +++ b/tests/test_encoding/test_encoding.py @@ -1,3 +1,4 @@ +import hashlib import json import os import subprocess @@ -24,6 +25,7 @@ class _TestCase: # Whether to compare output of command run. compare_output: bool = True compare_output_as_json: bool = False + golden_sha256: str = "" cc_optimization_arg: str = "" @@ -33,6 +35,10 @@ class _TestCase: def __post_init__(self) -> None: if not self.langs: self.langs = ["c", "go", "py"] + langs = os.environ.get("BP_TEST_LANGS", "") + if langs: + requested_langs = langs.split(",") + self.langs = [lang for lang in self.langs if lang in requested_langs] self.cc_optimization_arg = os.environ.get("BP_TEST_CC_OPTIMIZATION", "") self.optimization_mode_arg = os.environ.get("BP_TEST_OPTIMIZATION_ARG", "") @@ -67,11 +73,21 @@ def compare_outputs(self, outputs: List[bytes]) -> None: else: assert json.loads(out.strip()) == json.loads(outputs[0].strip()) + def compare_golden_output(self, outputs: List[bytes]) -> None: + """Compares encoded output bytes against a golden digest.""" + if not self.golden_sha256: + return + for out in outputs: + digest = hashlib.sha256(out.strip()).hexdigest() + assert digest == self.golden_sha256 + def run(self) -> None: """Run this case, returns the""" if self.optimization_mode_arg and not self.support_optimization_mode: return + if not self.langs: + return current_cwd = os.getcwd() outputs: List[bytes] = [] @@ -85,6 +101,7 @@ def run(self) -> None: if self.compare_output: self.compare_outputs(outputs) + self.compare_golden_output(outputs) finally: self.execute_cmd_clean() @@ -118,11 +135,17 @@ def test_encoding_nested() -> None: def test_encoding_arrays() -> None: - _TestCase("arrays").run() + _TestCase( + "arrays", + golden_sha256="97d5cf1251363d5de7af2c3586ae6581d12fc73e8608c87594e47c20a919aba5", + ).run() def test_encoding_scatter() -> None: - _TestCase("scatter").run() + _TestCase( + "scatter", + golden_sha256="ab428ea4324fb51fe2bf0e486b477e7f1880967520a8a07588755ed983f88496", + ).run() def test_encoding_enums() -> None: @@ -130,11 +153,18 @@ def test_encoding_enums() -> None: def test_encoding_signed() -> None: - _TestCase("signed").run() + _TestCase( + "signed", + golden_sha256="420d13682d2bec113d32b4504f29e01023b28c6391d275afd0eb16dfd7662e6a", + ).run() def test_encoding_complexx() -> None: - _TestCase("complexx", support_optimization_mode=False).run() + _TestCase( + "complexx", + support_optimization_mode=False, + golden_sha256="4e43481907688740db930c1b034cd746e7a72c6865e9076cfe080d48c1c6f1c2", + ).run() def test_encoding_issue52() -> None: diff --git a/tests/test_encoding/test_endian.py b/tests/test_encoding/test_endian.py index 7f074a0..f122742 100644 --- a/tests/test_encoding/test_endian.py +++ b/tests/test_encoding/test_endian.py @@ -300,6 +300,58 @@ def test_opt_mode_big_endian_branch(case: str) -> None: """ +_RUNTIME_SIGN_EXTENSION_TEST_C = r""" +#include +#include +#include "bitproto.h" + +int main(void) { + struct BpProcessorContext ctx = BpProcessorContext(false, 0); + + int8_t i7 = 0x7F; + BpHandleIntSignAfterEndecode(sizeof(i7), 7, &ctx, &i7); + assert(i7 == -1); + + int16_t i15 = 0x7FFE; + BpHandleIntSignAfterEndecode(sizeof(i15), 15, &ctx, &i15); + assert(i15 == -2); + + int32_t i31 = 0x7FFFFFFD; + BpHandleIntSignAfterEndecode(sizeof(i31), 31, &ctx, &i31); + assert(i31 == -3); + + int64_t i62 = ((int64_t)1 << 62) - 181818; + BpHandleIntSignAfterEndecode(sizeof(i62), 62, &ctx, &i62); + assert(i62 == -181818); + + int64_t positive_i62 = 181818; + BpHandleIntSignAfterEndecode(sizeof(positive_i62), 62, &ctx, &positive_i62); + assert(positive_i62 == 181818); + + return 0; +} +""" + + +def _compile_sign_extension_test(tmp_path, *cc_args: str): + src = tmp_path / "sign_extension_test.c" + src.write_text(_RUNTIME_SIGN_EXTENSION_TEST_C) + binpath = tmp_path / "sign_extension_test" + subprocess.check_call( + [ + "gcc", + *cc_args, + "-I", + LIB_C_DIR, + str(src), + os.path.join(LIB_C_DIR, "bitproto.c"), + "-o", + str(binpath), + ] + ) + return binpath + + @requires_tools def test_runtime_big_endian_layout(tmp_path) -> None: """The non-optimization runtime big-endian path maps big-endian field memory @@ -324,6 +376,27 @@ def test_runtime_big_endian_layout(tmp_path) -> None: subprocess.check_call([str(binpath)]) +@requires_tools +def test_runtime_sign_extension(tmp_path) -> None: + """Sign extension for arbitrary-width signed integers avoids shift UB.""" + binpath = _compile_sign_extension_test(tmp_path) + subprocess.check_call([str(binpath)]) + + +@requires_tools +def test_runtime_sign_extension_ubsan(tmp_path) -> None: + """Little-endian sign extension must not rely on undefined shifts.""" + try: + binpath = _compile_sign_extension_test( + tmp_path, + "-fsanitize=undefined", + "-fno-sanitize-recover=undefined", + ) + except subprocess.CalledProcessError as exc: + pytest.skip(f"gcc does not support UBSan for this target: {exc}") + subprocess.check_call([str(binpath)]) + + @requires_tools def test_runtime_big_endian_array(tmp_path) -> None: """BpEndecodeArray on big-endian processes elements independently (no