Skip to content

Use SafeUnpickler #2247

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

Merged
merged 9 commits into from
Jul 22, 2025
Merged

Use SafeUnpickler #2247

merged 9 commits into from
Jul 22, 2025

Conversation

yiliu30
Copy link
Contributor

@yiliu30 yiliu30 commented Jul 18, 2025

Type of Change

feature or bug fix or documentation or validation or others
API changed or not

Description

detail description

Expected Behavior & Potential Risk

the expected behavior that triggered by this PR

How has this PR been tested?

how to reproduce the test (including hardware information)

Dependency Change?

any library dependency introduced or removed

Signed-off-by: yiliu30 <[email protected]>
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a security improvement by replacing the unsafe pickle.load() with a custom SafeUnpickler class to prevent arbitrary code execution during deserialization of tuning history files.

  • Implements a custom SafeUnpickler class with restricted class loading
  • Replaces direct pickle.load() usage with the safer alternative
  • Adds necessary imports for builtins and OrderedDict

@yiliu30
Copy link
Contributor Author

yiliu30 commented Jul 21, 2025

@chensuyue @XuehaoSun
The CI failed due to the issue below. Can we get it merged first?

2025-07-21T11:05:58.8719458Z Installing collected packages: protobuf
2025-07-21T11:05:58.8719820Z   Attempting uninstall: protobuf
2025-07-21T11:05:58.8720027Z     Found existing installation: protobuf 3.19.6
2025-07-21T11:05:58.8757317Z     Uninstalling protobuf-3.19.6:
2025-07-21T11:05:59.0170443Z       Successfully uninstalled protobuf-3.19.6
2025-07-21T11:05:59.1161078Z ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
2025-07-21T11:05:59.1161556Z tensorboard 2.10.1 requires protobuf<3.20,>=3.9.2, but you have protobuf 3.20.3 which is incompatible.
2025-07-21T11:05:59.1161810Z tensorflow 2.11.dev202323 requires protobuf<3.20,>=3.9.2, but you have protobuf 3.20.3 which is incompatible.

@chensuyue
Copy link
Contributor

@chensuyue @XuehaoSun The CI failed due to the issue below. Can we get it merged first?

2025-07-21T11:05:58.8719458Z Installing collected packages: protobuf
2025-07-21T11:05:58.8719820Z   Attempting uninstall: protobuf
2025-07-21T11:05:58.8720027Z     Found existing installation: protobuf 3.19.6
2025-07-21T11:05:58.8757317Z     Uninstalling protobuf-3.19.6:
2025-07-21T11:05:59.0170443Z       Successfully uninstalled protobuf-3.19.6
2025-07-21T11:05:59.1161078Z ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
2025-07-21T11:05:59.1161556Z tensorboard 2.10.1 requires protobuf<3.20,>=3.9.2, but you have protobuf 3.20.3 which is incompatible.
2025-07-21T11:05:59.1161810Z tensorflow 2.11.dev202323 requires protobuf<3.20,>=3.9.2, but you have protobuf 3.20.3 which is incompatible.

This is why the test failed, but looks like your PR fix the issue. Because it's pass in PR test, failed in baseline.
image

@XuehaoSun XuehaoSun merged commit 0b63176 into master Jul 22, 2025
32 of 33 checks passed
@XuehaoSun XuehaoSun deleted the fix-tuning-his branch July 22, 2025 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants