Fix assorted issues with legacy systems #644

ilammy · 2020-05-25T17:23:41Z

Fix and work around miscellaneous issues found by BuildBot. It's unique in that it tests some rarely used systems like CentOS 7 and 32-bit machines which we don't test on CircleCI or GitHub Actions.

This should probably be separate PRs but I don't want to complicate things, wait for a gazillion of builds which don't really test these changes.

Avoid inline variables in "for" loops

Some systems (like CentOS 7) still run their C compilers in some C89 compatibility mode, so any C99 code will fail to build there. Use C89 style of variable declarations to avoid warnings treated as errors.

Do not use --no-undefined with Emscripten

It turns out that not all Emscripten toolchains support --no-undefined flag and complain about that by breaking builds, in particular on our BuildBot infrastructure.

We use --no-undefined to get warnings about missing dependencies which are usually caused by OpenSSL header files that do not match the library binaries actually linked. We use BoringSSL with Emscripten by default so we don't need those warnings that much here.

Drop layout tests from libthemis-sys

struct secure_session_user_callbacks_type and others include architecture-specific types such as pointers and size_t values.

PR #626 has replaced on-the-fly invocation of Bindgen with using pregenerated files. Unfortunately, we cannot use layout tests in these files since they end up architecture-specific.

On 64-bit machines – such as the one used to generate the files – secure_session_user_callbacks_type uses 40 bytes of storage while on 32-bit machines it requires only 20 bytes. We cannot include a test that simply compares the size with a constant "40".

I don't want to complicate things too much just because of those tests so let's simply drop them if they are the only platform-dependent part here.

Ignore panicking corruption tests in RustThemis and GoThemis

Token Protect data corruption test triggers a panic in Vec::reserve call on 32-bit machines. Corrupted input data is not detected right away by Themis and we try to allocate more than 2 GB of memory. It fails on 32-bit systems and Rust panics.

There is a nightly-only experimental API – Vec::try_reserve – but we need to support stable Rust.

We can't do much about this issue other than assume a safe allocation interval which is only an assumption. So for now let's panic, and disable this test on 32-bit machines so that the test suite is still useful.

Similar to Rust, GoThemis panics on allocation when trying to allocate slice of negative size (a side effect of casting big C.size_t to int on 32-bit systems).

This time this is a genuine overflow that we can and should avoid, but right now we don't have time to audit and update all C.size_t casts. Leave this issue as is and skip the faulty test for now.

We fixed the same issues for Java in #639 because if affects all systems. However, it's less likely that RustThemi and GoThemis will be used on 32-bit machines so we kinda can ignore these issues and suppress test failures. But yeah, this is not a fix, Themis will still panic when processing corrupted data on 32-bit machines.

Checklist

Change is covered by automated tests (somewhat, 64-bit only)
The coding guidelines are followed
~~Changelog is updated~~ (no need, IMO)

Some systems (like CentOS 7) still run their C compilers in some C89 compatibility mode, so any C99 code will fail to build there. Since we do not require C99, declare variables at the start of blocks. Note that we do use some other features like C++ comments so we cannot enable strict C89 compatibility mode. CentOS 7 uses "-std=gnu90" which includes various GNU extensions such as line comments, but not variable declarations in arbitrary places.

It turns out that not all Emscripten toolchains support "--no-undefined" flag and complain about that by breaking builds, in particular on our Buildbot infrastructure. We use "--no-undefined" to get warnings about missing dependencies which are usually caused by OpenSSL header files that do not match the library binaries actually linked. We use BoringSSL with Emscripten by default so we don't need those warnings that much here.

struct secure_session_user_callbacks_type and others include architecture-specific types such as pointers and size_t values. Commit 85af22d (Drop "bindgen" from libthemis-sys dependencies, 2020-04-25) has replaced on-the-fly invocation of Bindgen with using pregenerated files. Unfortunately, we cannot use layout tests in these files since they end up architecture-specific. On 64-bit machines -- such as the one used to generate the files -- secure_session_user_callbacks_type uses 40 bytes of storage while on 32-bit machiens is requires only 20 bytes. We cannot include a test that simply compares the size with a constant "40". I don't want to complicate things too much just because of those tests so let's simply drop them if *they* are the only platform-dependent part here.

This test triggers a panic in "Vec::reserve" call in Token Protect implementation on 32-bit machines. Corrupted input data is not detected right away by Themis and we try to allocate more than 2 GB of memory. It fails on 32-bit systems and Rust panics. There is a nightly-only experimental API -- Vec::try_reserve -- but we need to support stable Rust. We can't do much about this issue other than assume a safe allocation interval which is only an assumption. So for now let's panic, and disable this test on 32-bit machines so that the test suite is still useful.

Similar to Rust, GoThemis panics on allocation when trying to allocate slice of negative size (a side effect of casting big C.size_t to int on 32-bit systems). This time this is a genuine overflow that we can and should avoid, but right now we don't have time to audit and update all C.size_t casts. Leave this issue as is and skip the faulty test for now.

vixentael · 2020-05-25T20:16:06Z

gothemis/cell/token_protect_test.go

+	// FIXME(ilammy, 2020-05-25): avoid capacity allocation panics (T1648)
+	// This tests panics on 32-bit architectures due to "int" overflow.
+	// The implementation needs to check for "int" range when casting "C.size_t".
+	if uint64(^uint(0)) == uint64(^uint32(0)) {


have we started to disable tests because we don't like them? :P

(i've read PR description, just kidding here)

Oh, right. For posterity: a fix for this issue is scheduled for 0.13.1. After that decryption of corrupted data of this sort will return an error — likely a different one for 32-bit and 64-bit machines, but still an error.

vixentael · 2020-05-25T20:16:58Z

tests/themis/themis_secure_cell.c

@@ -1911,6 +1912,7 @@ static void scell_seal_passphrase_compatibility(void)
 static void scell_seal_passphrase_stability(void)
 {
    themis_status_t res = THEMIS_FAIL;
+    size_t i;


hello size_t, our old friend 👀

shadinua · 2020-05-26T07:37:56Z

src/wrappers/themis/rust/libthemis-sys/bindgen.sh

@@ -29,8 +29,12 @@ WHITELIST="(THEMIS|themis|secure_(comparator|session)|STATE)_.*"
 # thing to do is to split the generated code into different files for different
 # platforms (like themis_x86_64.rs, themis_arm64.rs, etc.) and conditionally
 # compile them depending on target.
+#
+# Though, we do disable layout tests which *are* architecture-specific and
+# we cannot include them without breaking either 32-bit or 64-bit machines.


Perhaps we could include tests depending on the current architecture?

Perhaps we could include tests depending on the current architecture?

We could but there are a lot of ifs here. Since Docker does not support 32-bit hosts, cloud-based CI services do not offer 32-bit runners. We probably could plug in our own into GitHub Actions, but I don't know whether their runner supports 32-bit systems. Ideally we'd be testing on ARM systems as well.

We can use Docker containers with 32-bit userland but that's not quite the same. Though, it'd probably catch the issues we have encountered here since they are more about integer overflow than memory limits.

Another thing to consider is that it would double the test matrix since we'd better be testing all wrappers, Core alone is not enough.

Overall, I'd like to get this done eventually. Right now we rely only on internal BuildBot runs to have something resembling regular testing on something other that x86_64.

Or do you mean that layout tests should be there, just be different for architectures?

if cfg!(target_pointer_size = "32") { // test for 32-bit } else if cfg!(target_pointer_size = "64") { // test for 64-bit } else { // dunno fail }

Well, I'd like Bindgen to be able to generate tests like that automatically, but that's quite hard. The best we could get right now is generate multiple modules:

src/x86_64-unknown-linux-gnu.rs

src/i686-unknown-linux-gnu.rs

src/aarch64-apple-ios.rs

...

And then conditionally build and reexport one of them.

However, since the only thing that is different is the tests, I feel that it's a little bit overkill.

Or do you mean that layout tests should be there, just be different for architectures?

Of course, it would be great to have a complete set of test for each of platforms.

Well, I share your view at all. Lets start from this position and have all our future plans in the backlog :)

Lets [...] have all our future plans in the backlog :)

Yup. I've just created an internal task to track this (T1655).

Lagovas · 2020-05-26T10:19:51Z

gothemis/cell/token_protect_test.go

+	// FIXME(ilammy, 2020-05-25): avoid capacity allocation panics (T1648)
+	// This tests panics on 32-bit architectures due to "int" overflow.
+	// The implementation needs to check for "int" range when casting "C.size_t".
+	if uint64(^uint(0)) == uint64(^uint32(0)) {


maybe better to split to two functions which test 32/64 bit versions and place them to separate files with specific build tags? One file will be compiled only if run on 32-bit system, another on 64-bit. Then we will not have such check and if somebody runs tests on their own 32 bit machine, tests will be checked.

The problem is that it fails on 32-bit machines and we don't want that so we temporarily disable the test. It's still there and will be shown as skipped.

I believe that Themis is a portable library so the test suite should not really be platform-dependent. The same tests should pass everywhere, regardless of pointer size, number of registers, endianness, etc. (Except for natural limitations, such as impossibility of handling continuous 3 GB data buffers on 32-bit machines.)

So I'd rather have this ugly wart reminding that it's an ugly wart instead of accepting different test suites for different architectures as a normal thing.

Lagovas

lgtm

ilammy added 5 commits May 25, 2020 20:04

ilammy added core Themis Core written in C, its packages W-GoThemis 🐹 Wrapper: GoThemis, Go API infrastructure Automated building and packaging W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages labels May 25, 2020

ilammy requested review from vixentael, Lagovas and shadinua May 25, 2020 17:23

Ignore one more test as well

15636a3

vixentael reviewed May 25, 2020

View reviewed changes

shadinua reviewed May 26, 2020

View reviewed changes

Lagovas reviewed May 26, 2020

View reviewed changes

Lagovas approved these changes May 26, 2020

View reviewed changes

shadinua approved these changes May 26, 2020

View reviewed changes

vixentael approved these changes May 26, 2020

View reviewed changes

ilammy merged commit d8a1c14 into master May 26, 2020

ilammy deleted the buildbot branch May 26, 2020 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix assorted issues with legacy systems #644

Fix assorted issues with legacy systems #644

ilammy commented May 25, 2020

vixentael May 25, 2020 •

edited

Loading

ilammy May 26, 2020

vixentael May 25, 2020

shadinua May 26, 2020

ilammy May 26, 2020

shadinua May 26, 2020 •

edited

Loading

ilammy May 26, 2020 •

edited

Loading

Lagovas May 26, 2020

ilammy May 26, 2020 •

edited

Loading

Lagovas left a comment

Fix assorted issues with legacy systems #644

Fix assorted issues with legacy systems #644

Conversation

ilammy commented May 25, 2020

Avoid inline variables in "for" loops

Do not use --no-undefined with Emscripten

Drop layout tests from libthemis-sys

Ignore panicking corruption tests in RustThemis and GoThemis

Checklist

vixentael May 25, 2020 • edited Loading

Choose a reason for hiding this comment

ilammy May 26, 2020

Choose a reason for hiding this comment

vixentael May 25, 2020

Choose a reason for hiding this comment

shadinua May 26, 2020

Choose a reason for hiding this comment

ilammy May 26, 2020

Choose a reason for hiding this comment

shadinua May 26, 2020 • edited Loading

Choose a reason for hiding this comment

ilammy May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Lagovas May 26, 2020

Choose a reason for hiding this comment

ilammy May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

vixentael May 25, 2020 •

edited

Loading

shadinua May 26, 2020 •

edited

Loading

ilammy May 26, 2020 •

edited

Loading

ilammy May 26, 2020 •

edited

Loading