Skip to content
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

Avoid UB in installer and check manifest data in advance #1628

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

InitAuther97
Copy link

In the latest release of Trials, modifying the forge installer jar file may corrupt the manifest data, exposing a potential issue of relying on an undefined order of ServiceLoader.load(...).stream() between different modules in the same layer.
The issue will cause a conditional crash that depends on the archive file name when the installer is modified externally. The module name will also be related to the file name.

In this PR:

  1. The manifest data is checked in advance and if it's corrupted (in this case nothing will be read), the installer crashes with a reminder.
  2. The service loading mechanism is replaced with reflection to ensure proper working.

@InitAuther97 InitAuther97 changed the title Avoid UB in launcher and check manifest data in advance Avoid UB in installer and check manifest data in advance Jan 20, 2025
var cw = new ClassWriter(ClassWriter.COMPUTE_MAXS);
asmClass.accept(cw);
var bytes = cw.toByteArray();
Unsafe.defineClass(cpwClassName, bytes, 0, bytes.length, cl, domain);
Copy link
Owner

Choose a reason for hiding this comment

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

Unsafe 现在已经 deprecated for removal 了,看能不能不用

https://openjdk.org/jeps/498

Copy link
Author

Choose a reason for hiding this comment

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

那就把这个做成一个ClassLoader

@InitAuther97
Copy link
Author

现在改成用ClassLoader拦截类加载了,但是有点取巧,把它的模块信息全扔掉了(不然要用cpw的ModuleClassLoader,需要自己处理所有库,就变成另一个BL了),标记了一下

@IzzelAliz IzzelAliz merged commit c1e6367 into IzzelAliz:Trials Jan 23, 2025
1 check passed
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