Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_serialize_4b_bytestring / test_serialize_8b_bytestring tests fail on 32-bit systems (size_t overflow) #263

Closed
trofi opened this issue Jan 7, 2023 · 2 comments · Fixed by #266
Assignees
Labels

Comments

@trofi
Copy link
Contributor

trofi commented Jan 7, 2023

Describe the bug

On i686-linux targets libcbor tests fail as:

[ RUN      ] test_serialize_4b_bytestring
[  ERROR   ] --- 0 != 0x100000004
[   LINE   ] --- libcbor/test/cbor_serialize_test.c:...: error: Failure!
[  FAILED  ] test_serialize_4b_bytestring
...
[  FAILED  ] test_serialize_8b_bytestring

I think it happens in this code: https://github.com/PJK/libcbor/blob/master/test/cbor_serialize_test.c#L161-L183

Annotated:

static void test_serialize_4b_bytestring(void **_CBOR_UNUSED(_state)) {
  cbor_item_t *item = cbor_new_definite_bytestring();

  // Fake having a huge chunk of data
  unsigned char *data = malloc(1);
  cbor_bytestring_set_handle(item, data, UINT32_MAX);

  assert_int_equal(cbor_serialize(item, buffer, 512), 0); // fails to serialze due to size_t overflow
  assert_int_equal(cbor_serialized_size(item), (uint64_t)UINT32_MAX + 5);
    // fails: cbor_serialized_size() returns size_t, can't return more than UINT32_MAX
    // `(uint64_t)UINT32_MAX + 5` always exceeds UINT32_MAX.
  cbor_decref(&item);
}

Aside: assert_int_equal() is a bit suspicious as it uses 32-bit int comparison. Is is intentional?

To Reproduce

On i686-linux run cmake .. && make test.

Expected behavior

tests should pass

Environment

  • libcbor version: 0.10.0, 0.10.1, git
  • and build configuration flags (or source package version if using a package manager): libcbor> cmake flags: -DCMAKE_FIND_USE_SYSTEM_PACKAGE_REGISTRY=OFF -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_LOCALEDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/share/locale -DCMAKE_INSTALL_LIBEXECDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/libexec -DCMAKE_INSTALL_LIBDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/lib -DCMAKE_INSTALL_DOCDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/share/doc/libcbor -DCMAKE_INSTALL_INFODIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/share/info -DCMAKE_INSTALL_MANDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/share/man -DCMAKE_INSTALL_OLDINCLUDEDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/include -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/include -DCMAKE_INSTALL_SBINDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/sbin -DCMAKE_INSTALL_BINDIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/bin -DCMAKE_INSTALL_NAME_DIR=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0/lib -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_OSX_SYSROOT= -DCMAKE_FIND_FRAMEWORK=LAST -DCMAKE_STRIP=/nix/store/9hi79llhgx643mrfc5sz85fz5kzamz7k-gcc-wrapper-13.0.0/bin/strip -DCMAKE_RANLIB=/nix/store/9hi79llhgx643mrfc5sz85fz5kzamz7k-gcc-wrapper-13.0.0/bin/ranlib -DCMAKE_AR=/nix/store/9hi79llhgx643mrfc5sz85fz5kzamz7k-gcc-wrapper-13.0.0/bin/ar -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_INSTALL_PREFIX=/nix/store/76k0gib6bagdgy8lc1j6m7rr9xn2qcik-libcbor-0.10.0 -DCMAKE_INSTALL_LIBDIR=lib -DBUILD_SHARED_LIBS=on -DWITH_TESTS=ON
@PJK
Copy link
Owner

PJK commented Jan 8, 2023

Indeed, you're right, that test is not portable. Thank you for the prompt bug report!

#264 should prevent future regressions

@PJK PJK closed this as completed in #266 Jan 8, 2023
@trofi
Copy link
Contributor Author

trofi commented Jan 8, 2023

master does pass all tests for my environment as well. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants