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

Unsafe review notes #98

Open
Manishearth opened this issue Feb 14, 2024 · 5 comments
Open

Unsafe review notes #98

Manishearth opened this issue Feb 14, 2024 · 5 comments

Comments

@Manishearth
Copy link
Contributor

Manishearth commented Feb 14, 2024

I'm performing an unsafe review of encoding_rs. The hope is by the end of this I will have safety documentation for all unsafe blocks in the crate.

Opening this issue to keep track of things I have found, especially things I do not plan to fix with documentation (but may fix later).

The branch with the documentation is https://github.com/manishearth/encoding_rs/tree/unsafe (compare)

@Manishearth
Copy link
Contributor Author

Existing UB: #79

@Manishearth
Copy link
Contributor Author

One thing I think that can be done is that the constants in ascii.rs can be cleaned up greatly.

I believe the following works:

pub const ALU_STRIDE_SIZE: usize = mem::size_of::<usize> * 2;
pub const MAX_STRIDE_SIZE: usize = ALU_STRIDE_SIZE;
pub const ALU_ALIGNMENT: usize = mem::align_of::<usize>();
pub const ALU_ALIGNMENT_MASK: usize = ALU_ALIGNMENT - 1;

For SIMD, I think we can do

pub const SIMD_ALIGNMENT = align_of::<u16x8>();

and assert in a constant that it is equivalent to align_of::<u8x16>()

@Manishearth
Copy link
Contributor Author

The store8_unaligned in copy_bmp_to could probably be aligned since we're writing to an aligned u16 slice

if SIMD_STRIDE_SIZE is a multiple of 4 (it is).

@Manishearth
Copy link
Contributor Author

Manishearth commented Feb 15, 2024

Utf16Destination/Utf8Destination are extremely leaky abstractions, safety wise. Their safety depends heavily on how, precisely, they are used, but they're not unsafe.

I'm not sure if they can be properly reviewed without adding unsafe there

@Manishearth
Copy link
Contributor Author

Manishearth commented Mar 6, 2024

I think the way to handle Utf16/Utf8Destination is to change the handle types to be "checked for one write" and "checked for two writes" (etc), and have them call each other. I think I'll do this as a separate PR, but I am convinced these are safe now.

They're fiddly enough that I think a refactoring would help in the long run. The invariants are currently scattered across the file.

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

No branches or pull requests

1 participant