Skip to content

Core: Add restricted_dumps helper #5117

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

duckboycool
Copy link
Contributor

What is this fixing or adding?

Adds a restricted_dumps method to Utils corresponding to restricted_loads which errors if the object cannot be unpickled with restricted_loads, and replaces some current uses of pickle.dumps with it. I'm not positive if some of these uses might be safe to not have the check, although they might also be able to remove the corresponding use of restricted_loads if so. There seemed to be no relevant uses of pickle.dump to a file instead, but it might be better to implement a RestrictedPickler class instead of just the function if the flexibility is desired.
I also changed the test_pickle_dumps test to also try getting the default with from_text since this value might be different than the one gotten with from_any(option.default) and cause issues.

How was this tested?

I tested the changes with a problem that was present in the satisfactory world options that caused it to error when generating a single player game from the options page. It fails immediately in this case instead of first trying to generate and then failing later when unpickling. I also ran the updated test on the world and it was able to find the error whereas the world passed the test before.

If this makes graphical changes, please attach screenshots.

🚫🥒

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 15, 2025
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@NewSoupVi
Copy link
Member

I'm gonna merge this at the start of next release cycle

Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

looked over code
local generated, served, and played a game

@Berserker66
Copy link
Member

This does make dumping quite a bit slower, and in some situations I feel it should never fail, like webhost upload. - where the increased performance cost would also be among the highest impacted.

@duckboycool
Copy link
Contributor Author

I think the two in process_multidata (and the TestTracker setup) should definitely be safe since they came from a call to decompress which uses restricted_loads in the first place. The ones on multisaves also seem like they could be fine. Do you have an idea about whether some restricted_loads calls might also be unneeded?

@beauxq
Copy link
Collaborator

beauxq commented Jun 24, 2025

This does make dumping quite a bit slower, and in some situations I feel it should never fail, like webhost upload. - where the increased performance cost would also be among the highest impacted.

I don't know which serialization you mean by "webhost upload".
Can you point out which restricted_dumps calls in the diff you're most concerned about for performance?

We've seen it fail in at least 3 of these places. But, yeah maybe some of them don't need it.

@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants