Fix some undefined behavior #1389
Conversation
Point was not layout compatible with sys::SDL_Point, making the transmute unsound. the same for Rect and sys::SDL_Rect. using repr(transparent) on the wrapper types fixes this.
sdl2-sys/sdl_bindings.rs
Outdated
| @@ -24387,7 +24387,7 @@ extern "C" { | |||
| dstrect: *const SDL_Rect, | |||
| angle: f64, | |||
| center: *const SDL_Point, | |||
| flip: SDL_RendererFlip, | |||
| flip: u32, | |||
There was a problem hiding this comment.
is this a good way to fix this issue? this file is (partly?) generated, but the original function signature did not allow SDL_FLIP_HORIZONTAL | SDL_FLIP_VERTICAL
There was a problem hiding this comment.
This file is always fully generated and never modified manually (plus this breaks past code using sdl2-sys raw), so please revert that and adapt.
There was a problem hiding this comment.
i reverted the change on this file, and the changed function signature is now inside the function that uses it
sdl2-sys/sdl_bindings.rs
Outdated
| @@ -24387,7 +24387,7 @@ extern "C" { | |||
| dstrect: *const SDL_Rect, | |||
| angle: f64, | |||
| center: *const SDL_Point, | |||
| flip: SDL_RendererFlip, | |||
| flip: u32, | |||
There was a problem hiding this comment.
This file is always fully generated and never modified manually (plus this breaks past code using sdl2-sys raw), so please revert that and adapt.
…s/sdl_bindings.rs
|
For the [repr(transparent)], I understand the reason so it's fine (maybe add a comment so that we understand why it's there) For the flip flag, you should not change the sdl_bindings manually, but I do believe it needs to be changed. You need to change the parameters of binding so that it generates what you want. Have a look at https://docs.rs/bindgen/0.53.3/bindgen/struct.Builder.html#method.bitfield_enum and When looking at the bindgen docs, remember that we are quite a few versions behind, it might be a good idea to update at some point, but it does not look like it's necessary here. Anyway, putting an extern "C" in rust-sdl2 itself is a big code smell and a big no-no. |
will add a comment there
i could not generate the bindings on the current version of bindgen used here, searching the issue online got me to a closed issue on the bindgen repo, the fix was included in 0.69 and there it works for me for the sdl_bindings.rs, but not yet for the image, ttf and gfx ones because those have different versions? from this code i assumed sdl version 2.0.20 was used, but i could not find that version number in all of the other libraries |
or use c string literal instead
antonilol
force-pushed
the
fixes-unsafe-operations-1
branch
from
9814e1c to
f39c9a4
Compare
|
i reverted the fix to copy_ex for now, as it would probably be needed (and at least easier for me) to do this after updating bindgen, and also to wrap up this pr |
|
if you have no comments/question left on this pr you may merge it |
|
i started with updating bindgen here: https://github.com/antonilol/rust-sdl2/tree/update-bindgen, if you wanted to do this already you can use that as a starting point how did you generate the bindings yourself? like from which header files, which versions of software used. i assume you did not get an error on the current version of bindgen used in the -sys crate |
review note: read_pixels had some indentation changed, word diff looks better to see what changed there
fix some undefined behavior (not all changes are)
f374447 removes an unnecessary call to an unsafe function and 2accf99 fixes a memory leak