transmuting Vec might lead to undefined behavior about drm-rs HOT 9 CLOSED

I have investigated further and here's what I found:

The alloc feature on bytemuck requires Rust 1.36, so that's two versions higher than the 1.34 they have without that feature.

Unfortunately the cast_vec from bytemuck requires the types to implement some traits (NoUninit and AnyBitPattern exactly, but ideally just Pod). There is only one type that needs Vec casting - the drm_ffi::drm_mode_modeinfo struct and its transparent wrappper Mode. I don't think messing with drm_ffi and adding bytemuck derives is the best idea here, so I have opened an issue and a PR to allow this functionality for repr(transparent) types.

Otherwise, there would need to be some bytemuck derives directly in the drm_ffi crate - this could be achieved by using the ParserCallbacks::add_derives method and adding a bytemuck derive directly onto the struct as needed.

About the array mapping - to avoid the 1.55 MSRV this would need to be switched back to using some kind of transmute. However, this causes problems with the handle types because bytemuck and orphan rules don't allow us to implement Pod for Option<Handle> so that won't let us cast the arrays of u32 into arrays of optional handles. For that I have also opened an issue but I'm not sure if that proposition is the best solution.

from drm-rs.

Both seem like good suggestions. Feel free to open a PR, I would gladly accept it.

from drm-rs.

I have opened a PR with these changed. Furthermore, I've been messing with my fork and decided to reduce the number of transmutes overall, just to feel a little bit better. Most of the time they can be avoided altogether, although with two caveats:

• It uses array::map which was added in Rust 1.55 - I don't know if you have any minimum rust version policy
• It might have an effect on performance, depending on how the compiler decides to optimize it, but hopefully those paths aren't that hot anyway (plus compared to libdrm, which mallocs all over the place, I think this is still good).

The diff is here (it includes the PR I just opened as well) develop...TheEdward162:feature/reduce-transmutes

Would you be interested in merging this as well? If not I'll keep it in my fork.

from drm-rs.

We currently don't have a minimum version policy, but given that the crate currently is very conservative in it's use of newer APIs, I would bet some projects dependent on drm-rs might have.

I think a lot of your improvements should still apply without array::map, no?
For merging a PR with array::map we should probably have a discussion on adding a MSRV to the crate first and likely tag a new major-release just to be safe.

from drm-rs.

Many of them would, yes. Also, in this case, I think array::map could be emulated by a macro in most places.

from drm-rs.

I think the bytemuck crate would work with arrays and slices as well. Their MSRV is 1.34.

from drm-rs.

I think the bytemuck crate would work with arrays and slices as well. Their MSRV is 1.34.

1.34 sounds good enough, I think that is definitely a reasonable target. 👍

from drm-rs.

the 1.36 is for alloc stuff is the absolute minimum I'm afraid. Global allocator stuff wasn't stabilized until 1.36

from drm-rs.

1.36 would also be fine in my book. Just bumping to 1.55 I feel like would alienate a lot of users, given that the drm-api is also used in a bunch of embedded contexts, which can't that easily upgrade rust versions.

from drm-rs.