Skip to content

Conversation

@kampffrosch94
Copy link

Summary

This PR moves the AudioContext out of the overall Context.

Motivation

As discussed in #333 the current versions of get_context() and get_quad_context() and their uses rely on undefined behavior.
While I disagree that this is an issue that warrants a security advisory, I do think that we need to move to a sound solution.
The problem as I see it is that with some future version of rustc or LLVM macroquad might just straight up stop working.

Goals

  • fix the soundness issues by moving to safe alternatives
  • preserve the current API for user facing functions as much as possible
  • do it in a way that can be implemented and tested incrementally

Approach

Macroquad functions will not use get_context() anymore, but instead do their context dependent stuff inside a closure provided to with_context(..). with_context(..) internally borrows and unwraps a threadlocal RefCell<Option<Context>> and provides a &mut Context to the closure.

If multiple functions in the callstack try to borrow the context with_context() will panic.
To deal with this some public functions are split into a variant that borrows context and one that takes a &mut Context as argument.


While I was implementing above across all of macroquad I noticed that the audiomodule is only very lightly coupled to the rest of the Context.

So I decided to spin the AudioContext out into its own threadlocal refcell and put it into its own PR.

Changes in this PR

The AudioContext is now no longer part of Context. It lives in its own threadlocal RefCell<Option<QuadSndContext>>.
It is intialized via init_sound() just after the regular context is constructed.
It can be used via with_audio_context()

The userfacing API did not change.


I am working on a lot more than just the AudioContext over at https://github.com/kampffrosch94/macroquad_cell/tree/with_context.
But I'd like to validate my approach before I drop a multi thousand line PR 😅

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

Successfully merging this pull request may close these issues.

1 participant