Skip to content

fix: Sandboxing Scripts in Shared Contexts #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from

Conversation

Peepo-Juice
Copy link
Contributor

Summary

This PR changes the way scripts are loaded and how event handlers are called so that scripts don't collide when sharing the same context. Works to solve #339.

Load

  • Before: We would simply load the script without setting an environment, thus populating the global scope of the language context.
  • Now: An environment is created and set when loading the script, thus populating this new environment instead of the global scope. We are also injecting the context's globals into this environment, thus maintaining the script's access to the globals it expects to have. We store this new environment in a ScriptEnvironmentStore.

Function Calls

  • Before: We would search the context's globals for the requested function and then call it.
  • Now: We grab the environment from the ScriptEnvironmentStore via ScriptId, search for the requested function in that environment and then call it.

Next Steps

  • We need to apply this for Rhai and Rune, so far I only did Lua
  • Discuss whether we should apply this sand-boxing all the time, or only when shared context is enabled? Currently its doing it all the time, which didn't break anything for me personally when I turned off context sharing, but might have other implications I'm not aware of.
  • Write tests to ensure that two scripts that have the same event handlers can be triggered at will with no collisions, and still have access to data stored as globals (including BMS type registrations and other bindings)
  • Cleanup, I don't think I did a good job with the documentation.

@Peepo-Juice Peepo-Juice changed the title Sandboxing Scripts for Shared Context [fix] Sandboxing Scripts for Shared Context Mar 7, 2025
@Peepo-Juice Peepo-Juice changed the title [fix] Sandboxing Scripts for Shared Context fix Sandboxing Scripts in Shared Contexts Mar 7, 2025
@Peepo-Juice Peepo-Juice changed the title fix Sandboxing Scripts in Shared Contexts fix: Sandboxing Scripts in Shared Contexts Mar 7, 2025
@Peepo-Juice
Copy link
Contributor Author

Peepo-Juice commented Mar 8, 2025

Regarding the "when to sandbox": before my changes, the globals of scripts were accessible by every other script when context sharing was disabled. So this causes a "breaking change" in that respect. However, @makspll mentioned in Discord that your stance was that scripts should be private by default, but you may have meant strictly for shared contexts.

@@ -18,6 +18,7 @@ use bevy::{
reflect::TypePath,
utils::HashMap,
};
use mlua::Table;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙅 This is introducing lua logic into the bms_core layer. The idea is to only keep language agnostic logic in here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crate does have mlua and rhai dependencies behind feature flags, but those are only there to implemenet Error conversions

@Peepo-Juice
Copy link
Contributor Author

I will be fixing the issue of event handler collisions via a different method. I'll let someone else work on this later :)

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.

2 participants