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.

Sign up for GitHub

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

Jump to bottom

Fix assorted issues with legacy systems #644

Merged
merged 6 commits into from
May 26, 2020
Merged

Fix assorted issues with legacy systems #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f7dbe4
Avoid inline variables in "for" loops
ilammy May 25, 2020
80524bb
Do not use --no-undefined with Emscripten
ilammy May 25, 2020
0edcbc9
Drop layout tests from libthemis-sys
ilammy May 25, 2020
f8a911c
Ignore panicking corruption test in RustThemis
ilammy May 25, 2020
5d65d2b
Ignore panicking corruption test in GoThemis
ilammy May 25, 2020
15636a3
Ignore one more test as well
ilammy May 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ ifndef WITH_ASAN
ifndef WITH_MSAN
ifndef WITH_TSAN
ifndef WITH_UBSAN
# Not all Emscripten toolchains support these flags so leave them out as well.
ifndef IS_EMSCRIPTEN
ifdef IS_MACOS
LDFLAGS += -Wl,-undefined,error
endif
Expand All @@ -209,6 +211,7 @@ endif
endif
endif
endif
endif

CFLAGS += -O2 -g
# Get better runtime backtraces by preserving the frame pointer. This eats
Expand Down
14 changes: 14 additions & 0 deletions gothemis/cell/token_protect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,13 @@ func TestTokenProtectDetectExtendedData(t *testing.T) {
}

func TestTokenProtectDetectCorruptedToken(t *testing.T) {
// 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)) {
Copy link
Contributor

@vixentael vixentael May 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

t.Skip("avoid panic on 32-bit machines")
}

key, err := keys.NewSymmetricKey()
if err != nil {
t.Fatal("cannot generate symmetric key", err)
Expand Down Expand Up @@ -442,6 +449,13 @@ func TestTokenProtectDetectExtendedToken(t *testing.T) {
}

func TestTokenProtectSwapDataAndToken(t *testing.T) {
// 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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ilammy ilammy May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

t.Skip("avoid panic on 32-bit machines")
}

key, err := keys.NewSymmetricKey()
if err != nil {
t.Fatal("cannot generate symmetric key", err)
Expand Down
4 changes: 4 additions & 0 deletions src/wrappers/themis/rust/libthemis-sys/bindgen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could include tests depending on the current architecture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@shadinua shadinua May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

@ilammy ilammy May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

bindgen bindgen.h \
--no-doc-comments \
--no-layout-tests \
--rustified-enum "themis_key_kind" \
--size_t-is-usize \
--whitelist-function "$WHITELIST" \
Expand Down
166 changes: 0 additions & 166 deletions src/wrappers/themis/rust/libthemis-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,87 +326,6 @@ pub struct secure_session_user_callbacks_type {
pub get_public_key_for_id: get_public_key_for_id_callback,
pub user_data: *mut ::std::os::raw::c_void,
}
#[test]
fn bindgen_test_layout_secure_session_user_callbacks_type() {
assert_eq!(
::std::mem::size_of::<secure_session_user_callbacks_type>(),
40usize,
concat!("Size of: ", stringify!(secure_session_user_callbacks_type))
);
assert_eq!(
::std::mem::align_of::<secure_session_user_callbacks_type>(),
8usize,
concat!(
"Alignment of ",
stringify!(secure_session_user_callbacks_type)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_user_callbacks_type>())).send_data as *const _
as usize
},
0usize,
concat!(
"Offset of field: ",
stringify!(secure_session_user_callbacks_type),
"::",
stringify!(send_data)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_user_callbacks_type>())).receive_data as *const _
as usize
},
8usize,
concat!(
"Offset of field: ",
stringify!(secure_session_user_callbacks_type),
"::",
stringify!(receive_data)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_user_callbacks_type>())).state_changed as *const _
as usize
},
16usize,
concat!(
"Offset of field: ",
stringify!(secure_session_user_callbacks_type),
"::",
stringify!(state_changed)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_user_callbacks_type>())).get_public_key_for_id
as *const _ as usize
},
24usize,
concat!(
"Offset of field: ",
stringify!(secure_session_user_callbacks_type),
"::",
stringify!(get_public_key_for_id)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_user_callbacks_type>())).user_data as *const _
as usize
},
32usize,
concat!(
"Offset of field: ",
stringify!(secure_session_user_callbacks_type),
"::",
stringify!(user_data)
)
);
}
pub type secure_session_user_callbacks_t = secure_session_user_callbacks_type;
#[repr(C)]
#[derive(Debug, Copy, Clone)]
Expand All @@ -418,91 +337,6 @@ pub struct secure_session_peer_type {
pub sign_key: *mut u8,
pub sign_key_length: usize,
}
#[test]
fn bindgen_test_layout_secure_session_peer_type() {
assert_eq!(
::std::mem::size_of::<secure_session_peer_type>(),
48usize,
concat!("Size of: ", stringify!(secure_session_peer_type))
);
assert_eq!(
::std::mem::align_of::<secure_session_peer_type>(),
8usize,
concat!("Alignment of ", stringify!(secure_session_peer_type))
);
assert_eq!(
unsafe { &(*(::std::ptr::null::<secure_session_peer_type>())).id as *const _ as usize },
0usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(id)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_peer_type>())).id_length as *const _ as usize
},
8usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(id_length)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_peer_type>())).ecdh_key as *const _ as usize
},
16usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(ecdh_key)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_peer_type>())).ecdh_key_length as *const _
as usize
},
24usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(ecdh_key_length)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_peer_type>())).sign_key as *const _ as usize
},
32usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(sign_key)
)
);
assert_eq!(
unsafe {
&(*(::std::ptr::null::<secure_session_peer_type>())).sign_key_length as *const _
as usize
},
40usize,
concat!(
"Offset of field: ",
stringify!(secure_session_peer_type),
"::",
stringify!(sign_key_length)
)
);
}
pub type secure_session_peer_t = secure_session_peer_type;
extern "C" {
pub fn secure_session_peer_init(
Expand Down
10 changes: 10 additions & 0 deletions tests/rust/secure_cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,11 @@ mod token_protect {
}

#[test]
// FIXME(ilammy, 2020-05-25): avoid capacity allocation panics (T1649)
// This tests panics on 32-bit architectures due to size overflow.
// The implementation needs to use Vec::try_reserve instead of Vec::reserve
// when it becomes available in stable Rust.
#[cfg_attr(target_pointer_width = "32", ignore)]
fn detects_corrupted_token() {
let cell = SecureCell::with_key(SymmetricKey::new())
.unwrap()
Expand Down Expand Up @@ -810,6 +815,11 @@ mod token_protect {
}

#[test]
// FIXME(ilammy, 2020-05-25): avoid capacity allocation panics (T1649)
// This tests panics on 32-bit architectures due to size overflow.
// The implementation needs to use Vec::try_reserve instead of Vec::reserve
// when it becomes available in stable Rust.
#[cfg_attr(target_pointer_width = "32", ignore)]
fn detects_data_token_swap() {
let cell = SecureCell::with_key(SymmetricKey::new())
.unwrap()
Expand Down
6 changes: 4 additions & 2 deletions tests/themis/themis_secure_cell.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ static char user_context[] = "secure cell user context";

static bool non_zero_buffer(const uint8_t* buffer, size_t length)
{
for (size_t i = 0; i < length; i++) {
size_t i;
for (i = 0; i < length; i++) {
if (buffer[i] != 0) {
return true;
}
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

hello size_t, our old friend 👀

const uint8_t passphrase[] = "never gonna give you up";
const uint8_t message[] = "never gonna let you down";
const uint8_t context[] = "never gonna run around and desert you";
Expand All @@ -1931,7 +1933,7 @@ static void scell_seal_passphrase_stability(void)
(const uint8_t*)"\x80\x00\x01\x41\x0C\x00\x00\x00\x10\x00\x00\x00\x19\x00\x00\x00\x16\x00\x00\x00\x46\xAF\xD7\xFE\x56\xEE\x07\xD7\xA6\x40\x48\xA8\x00\xA9\xD6\x0C\x80\x67\x57\x65\xFF\xB6\x59\xD8\xC8\x77\xD8\x17\x40\x0D\x03\x00\x10\x00\xC3\xA1\x85\x9A\x13\x82\x7A\xEE\x1F\xDB\x9C\xB3\x31\x4E\xF9\x27\x3F\xAC\x61\x1C\xA5\x7D\xAA\x54\x43\xF3\x78\x19\x39\x7C\x5A\x9D\x71\x42\xA4\x6F\x87\x96\x1A\xBD\xDB"},
};

for (size_t i = 0; i < ARRAY_SIZE(encrypted_data); i++) {
for (i = 0; i < ARRAY_SIZE(encrypted_data); i++) {
uint8_t* decrypted = NULL;
size_t decrypted_length = 0;

Expand Down