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

Uses of slice::from_raw_parts with input from FFI would need some manual pre-checks #143

Open
glandium opened this issue Aug 14, 2024 · 2 comments

Comments

@glandium
Copy link

The preconditions for slice::from_raw_parts contain the following:

data must be non-null and aligned even for zero-length slices. One reason for this is that enum layout optimizations may rely on references (including slices of any length) being aligned and non-null to distinguish them from other data. You can obtain a pointer that is usable as data for zero-length slices using NonNull::dangling().

When the preconditions are not fulfilled, a debug build will fail with the message

unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed isize::MAX) at library\core\src\panicking.rs:220

and a release build may crash depending on what kind of optimizations the rust compiler allowed itself.

One way such thing can happen is when using the crc32 function like the following:

crc32(0, NULL, 0)

All the uses of slice::from_raw_parts should be audited as to whether the pointer/length pairs can originate from API user input in a similar way, and adjusted to make the preconditions met.

@folkertdev
Copy link
Collaborator

#145 fixes the immediate issue with crc32/adler32 and NULL. The NULL input is actually useful there because it gives the initial checksum value.

I'm working on other public-facing calls to unsafe functions (core::ptr::read and the from_raw_parts calls) and adding # Safety docs for the remaining functions.

In most cases the resonsibility of satisfying the preconditions just gets passed on to the caller, but for NULL in particular there is often (but not always) a special error that gets returned. So this requires some care to make sure we actually mirror the error output, and I plan to also return errors when stock zlib segfaults on NULL pointers.

@folkertdev
Copy link
Collaborator

On main, the public C api functions now all have a # Safety section that documents the assumptions the function makes. We also now have 100% branch coverage in that file, so edge cases (like NULL pointers) should be checked.

We return Z_STREAM_ERROR in some cases where stock zlib segfaults. Segfaults indicate UB so I believe that us returning a value is within the contract of the function.

Of course, these changes benefit from a second pair of eyes going over the assumptions.

Then there is of course also unsafe code internal to the library, but that already got tested (with miri too)/fuzzed a lot more.

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

2 participants