search

jeremyletang / rust-sfml

SFML bindings for Rust
Other
638 stars 88 forks source link

Fixed unsoundness in Music constructors #240

Closed Kolsky closed 3 years ago

Kolsky commented 3 years ago

Has a tiny amount of breaking changes, has not tested yet

239

Kolsky commented 3 years ago

Update: memory impl works fine, but the stream one fails with either STATUS_STACK_BUFFER_OVERRUN or the other one.

Kolsky commented 3 years ago

Still doesn't quite gets it. At least STATUS_ACCESS_VIOLATION seems to be gone, maybe there is something else with the library or my implementation.

crumblingstatue commented 3 years ago

Thank you for your work, I'll review it tomorrow

Kolsky commented 3 years ago

Here is my stack trace after calling ffi::sfMusic_destroy with a stream, just in case image

crumblingstatue commented 3 years ago

Can you post code that reproduces the error with my branch? I don't understand what's causing the problem, so it would be nice to have an example.

I'm going to bed now, but I'll continue investigating this tomorrow. Thank you for your hard work!

Kolsky commented 3 years ago

https://github.com/Kolsky/sfml_open_music Oddly enough, there is no need for the box, maybe C interface does some shenanigans? Idk what's real anymore :( Btw, it does reproduce the bug, so feel free to investigate.

crumblingstatue commented 3 years ago

Hmmm.... Unfortunately, I can't seem to reproduce the bug with that example. I tried all the Load variants, and run the program under valgrind, but it didn't find anything. But I'll try investigating further.

crumblingstatue commented 3 years ago

I fixed a bug in inputstream. Let me know if the issue still happens. https://github.com/jeremyletang/rust-sfml/commit/44dd1eb4630fe4af24d965ab6166da8de4c4b966

Kolsky commented 3 years ago

To be honest, it was already patched out in the experimental branch. Could've solved the case with ACCESS_VIOLATION, but unfortunately it's not the source of this particular behaviour. image I'm under stable-x86_64-pc-windows-msvc toolchain with SFML and CSFML 2.5 x64.

Edit: updated to the recent version of SFML, no luck.

crumblingstatue commented 3 years ago

https://github.com/jeremyletang/rust-sfml/commit/e4f1149554735f87471c75cbb2424d81f05de678 should have fixed the STATUS_STACK_BUFFER_OVERRUN problem. But let me know if you have additional problems, the stream API might not be safe just yet.

crumblingstatue commented 3 years ago

A lot of things have changed since this pull request was made, so I'm closing it. But thank you very much for your work! Don't take this as discouragement to submit future PRs!