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

feat: forbid most collections from containing zst elements/keys #202

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Aug 21, 2023

Part of #51 . (initially change caused by RUSTSEC-2023-0033)
Addressing review comment:
tmp

VecDeque, LinkedList, HashSet and BTreeSet have all been performing zst check during deserialization implicitly.
This pr adds corresponding checks to their serialization methods for consistency.

HashMap and BTreeMap have been performing this implicit check for (K, V) type.
A more strict check for only the key type in maps was added in the pr according to the mention in comment.

Check for arrays wasn't added, as there is a test, which asserts, that zsts are acceptable.

@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Aug 21, 2023

I've attempted to also address the following comment from review:

tmp

i've tried

fn fail_for_zst<T>() {
    const a: usize = size_of::<T>();
    static_assertions::const_assert_ne!(a, 0);
}

but hit a compilation error

@dj8yfo dj8yfo marked this pull request as ready for review August 21, 2023 17:51
@dj8yfo dj8yfo requested a review from frol as a code owner August 21, 2023 17:51
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Looks good to me!

borsh/tests/test_zero_sized_types.rs Outdated Show resolved Hide resolved
@dj8yfo dj8yfo force-pushed the zst_compile_time branch 2 times, most recently from 19fbf12 to 1902665 Compare August 24, 2023 18:23
@dj8yfo dj8yfo merged commit 189a021 into master Aug 24, 2023
7 checks passed
@dj8yfo dj8yfo deleted the zst_compile_time branch August 24, 2023 18:32
@frol frol mentioned this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants