Address unleaking of heap data causing miri test failures

[jamesmunns]

Fixes #74

It seems that the changes made to tests in #71 to unleak the heap buffers and allow disabling -Zmiri-ignore-leaks did so in a way that now makes miri angery.

This is currently a minimal commit disabling the leak step (in a hacky way). I'm going to investigate if there is a sound way to do this.

[jamesmunns]

@phil-opp this is now ready for review! I was able to restore the leak checks, the root cause here was that we were previously keeping a live (edit: and aliasing!) reference to the heap allocation in order to drop it in the closure.

I'm also... not sure how the closure-to-unleak previously worked? Seeing as the closure was never explicitly called. This came back to bite me in 3f8f9fc. I've gone ahead and made this a type with a Drop impl instead of a closure so that we know it will be called now. Let me know if I missed something on this.

edit: The fix is to not keep a reference, and instead allow for aliasing pointers instead. As long as the heap itself is dropped before we begin re-using the pointer (to unleak and drop the allocation), this is sound as per stacked borrows.

In general, we need to be really careful about ever mixing references and pointers.

[phil-opp]

Looks very good! Thanks a lot for investigating and fixing this so quickly!

[phil-opp]

I'm also... not sure how the closure-to-unleak previously worked? Seeing as the closure was never explicitly called. This came back to bite me in 3f8f9fc.

The closure was not called, but it owned the Box. So the memory was freed again when the closure was dropped. Clippy was okay with this approach, but apparently that was just a false negative.

I've gone ahead and made this a type with a Drop impl instead of a closure so that we know it will be called now. Let me know if I missed something on this.

That seems like a much clearer solution!

edit: The fix is to not keep a reference, and instead allow for aliasing pointers instead. As long as the heap itself is dropped before we begin re-using the pointer (to unleak and drop the allocation), this is sound as per stacked borrows.

In general, we need to be really careful about ever mixing references and pointers.

Makes sense. Agreed!

[phil-opp]

Given that this only affects the test framework, we should not need a new crates.io release for this, right?

[jamesmunns]

Yup, as far as I can tell this failure was only an issue in the test code itself, not the library.

…

On Mon, Nov 14, 2022, at 16:55, Philipp Oppermann wrote: Given that this only affects the test framework, we should not need a new crates.io release for this, right? — Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQAGLA5PESLJEXBH4AQJM3WIJOFNANCNFSM6AAAAAAR7YGOBM>. You are receiving this because you authored the thread.Message ID: ***@***.***>