Commit changes#5
Merged
Merged
Conversation
Override '-Djava.home', '-Dsun.boot.library.path', and '-Djava.library.path' to prevent crashes. Co-authored-by: PythonChicken123 <101510622+PythonChicken123@users.noreply.github.com>
Prefer on-disk PE parsing and add bounds checks to prevent access violations. Co-authored-by: PythonChicken123 <101510622+PythonChicken123@users.noreply.github.com>
Co-authored-by: PythonChicken123 <101510622+PythonChicken123@users.noreply.github.com>
Update in-memory walk match condition for wildcard and patch IAT. Co-authored-by: PythonChicken123 <101510622+PythonChicken123@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideRefactors and hardens the Windows JvmBootGlue PE/IAT handling and JVM launch path configuration: fixes the manual PE import walk, adds a disk-based import parser and api-set–aware IAT lookup, introduces new GetModuleHandle/GetModuleHandleEx hooks that correctly recognize the in‑memory jvm.dll, and enforces consistent java.home / boot library paths and environment when launching Minecraft in both OS-loader and memory-resident modes. Sequence diagram for GetModuleHandleEx hooks resolving in-memory jvm.dllsequenceDiagram
actor HotSpot
participant jvm_dll as jvm_dll
participant IAT_gmhew as IAT_gmhew_hook
participant IAT_gmfw as IAT_gmfw_hook
participant Kernel32 as kernel32
HotSpot->>jvm_dll: call os::init_system_properties_values
jvm_dll->>IAT_gmhew: GetModuleHandleExW(FLAG_FROM_ADDRESS, &os::init_system_properties_values, &h)
IAT_gmhew->>IAT_gmhew: check flags & FLAG_FROM_ADDRESS
IAT_gmhew->>IAT_gmhew: addr in [jvm_lo, jvm_hi)?
alt address_inside_jvm_range
IAT_gmhew-->>jvm_dll: return TRUE, *h = fake
else address_outside_jvm_range
IAT_gmhew->>Kernel32: real GetModuleHandleExW(flags, name_or_addr, out_h)
Kernel32-->>IAT_gmhew: result
IAT_gmhew-->>jvm_dll: propagate result
end
HotSpot->>jvm_dll: if (!h) return
jvm_dll->>IAT_gmfw: GetModuleFileNameW(h, home_path, MAX_PATH)
IAT_gmfw->>IAT_gmfw: if h == fake
IAT_gmfw-->>jvm_dll: return on_disk_jvm_path
jvm_dll-->>HotSpot: java.home initialised from on_disk_jvm_path
Sequence diagram for launch_minecraft JVM boot pathssequenceDiagram
actor Caller
participant launch as launch_minecraft
participant Adapter as adapter
participant Loader as JvmMemoryLoader
participant JvmBootGlue as JvmBootGlue
participant JPype as jpype
Caller->>launch: launch_minecraft(jdk_bin, memory_resident, extra_jvm_flags)
launch->>Adapter: resolve()
Adapter-->>launch: jvm_flags, classpath, main_class, game_args
launch->>launch: maybe override jdk_bin with adapter.detected_jdk_bin
launch->>launch: merge extra_jvm_flags
launch->>launch: jvm_flags = _enforce_jvm_path_overrides(jvm_flags, jdk_bin, require_runtime_image)
launch->>launch: _dll_search_cookies = _augment_dll_search_path(jdk_bin)
launch->>launch: java_home = _java_home_from_bin(jdk_bin)
launch->>_ScrubJvmEnv: __enter__(java_home)
note over launch,_ScrubJvmEnv: JVM env vars scrubbed, JAVA_HOME pinned
alt memory_resident == false
launch->>JPype: startJVM(jvm_dll_path, *jvm_flags, classpath)
JPype-->>launch: JVM started
launch->>JPype: JClass(main_class).main(game_args)
JPype-->>launch: game returns
launch->>launch: _wait_non_daemon_threads()
launch->>_ScrubJvmEnv: __exit__()
_ScrubJvmEnv-->>launch: env restored
launch-->>Caller: return
else memory_resident == true
Caller->>Loader: prepare memory-resident jvm.dll
Loader-->>launch: loader with fake handle
launch->>JvmBootGlue: __init__(loader, jvm_flags, classpath, main_class, game_args)
launch->>JvmBootGlue: boot()
JvmBootGlue-->>launch: JVM started, game finished
launch->>_ScrubJvmEnv: __exit__()
_ScrubJvmEnv-->>launch: env restored
launch-->>Caller: return
end
Class diagram for JvmBootGlue helpers and environment scrubbingclassDiagram
class JvmBootGlue {
- dict~str, _IATEntry|None~ _iat
- int _jvm_lo
- int _jvm_hi
- dict~str, object~ _hooks
+ _make_hooks() void
+ _patch_iat() void
+ boot() void
}
class PEImportCache {
+ dict~tuple~str, bytes~, int~ _PE_IMPORTS_CACHE
+ _imports_for_path(module_path: str) dict~tuple~str, bytes~, int~
+ _parse_pe_imports(data: bytes) dict~tuple~str, bytes~, int~
}
class IATLookup {
+ _find_iat(module_base: int, target_dll: str|None, target_fn: str, module_path: str|None) _IATEntry
}
class JvmPathEnforcer {
+ tuple~str~ _PINNED_PROPS
+ tuple~str~ _SCRUBBED_ENV_VARS
+ _java_home_from_bin(jdk_bin: Path) Path
+ _verify_runtime_image(java_home: Path, fatal: bool) bool
+ _enforce_jvm_path_overrides(jvm_flags: list~str~, jdk_bin: Path, require_runtime_image: bool) list~str~
+ _augment_dll_search_path(jdk_bin: Path) list~object~
}
class _ScrubJvmEnv {
- str _java_home
- dict~str, str|None~ _saved
+ __init__(java_home: Path)
+ __enter__() _ScrubJvmEnv
+ __exit__(*exc) void
}
class Launcher {
+ launch_minecraft(jdk_bin: Path, memory_resident: bool, extra_jvm_flags: list~str|None~) void
}
JvmBootGlue ..> PEImportCache : uses
JvmBootGlue ..> IATLookup : uses
Launcher ..> JvmPathEnforcer : uses
Launcher ..> _ScrubJvmEnv : uses
Launcher ..> JvmBootGlue : creates
PEImportCache ..> IATLookup : supplies imports for
JvmPathEnforcer ..> JvmBootGlue : ensures consistent java.home for
_ScrubJvmEnv ..> JvmPathEnforcer : pins JAVA_HOME based on
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Some of the section-divider comment lines now contain mojibake characters (e.g.
# ═════════════��/# ════════════��), which makes the file look corrupted; consider fixing these back to plain ASCII or valid Unicode separators for readability. - In
_safe_cstr,IsBadReadPtris configured on every call and relies on a deprecated API; consider initialising the function’sargtypes/restypeonce at module scope (or replacing it with aVirtualQuery/try-read approach) to reduce overhead and avoid repeated global state mutation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Some of the section-divider comment lines now contain mojibake characters (e.g. `# ═════════════��` / `# ════════════��`), which makes the file look corrupted; consider fixing these back to plain ASCII or valid Unicode separators for readability.
- In `_safe_cstr`, `IsBadReadPtr` is configured on every call and relies on a deprecated API; consider initialising the function’s `argtypes`/`restype` once at module scope (or replacing it with a `VirtualQuery`/try-read approach) to reduce overhead and avoid repeated global state mutation.
## Individual Comments
### Comment 1
<location path="scripts/jvm_boot_glue.py" line_range="480-483" />
<code_context>
- dname = ctypes.string_at(module_base + nrv).decode("ascii", "replace").upper()
- if dname == tdll:
+
+ dname = _safe_cstr(module_base + nrv).decode("ascii", "replace").upper()
+ if tdll is None or dname == tdll:
+ thunk = oft or ft # fall back to FT for bound imports
i = 0
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Empty strings from _safe_cstr on unreadable memory are treated as valid DLL names when target_dll is None.
Because `_safe_cstr` returns `b""` on unreadable memory, you end up treating an empty DLL name as a match when `target_dll is None` and then walking its thunk table. For malformed images this adds unnecessary work, can surface extra `OSError`s, and makes the matching semantics less clear. Consider skipping descriptors with empty names (e.g. `if not dname: desc += 20; continue`) so only readable, non-empty names are included in the wildcard search.
```suggestion
raw_dname = _safe_cstr(module_base + nrv)
if not raw_dname:
# Skip unreadable or empty DLL names when doing wildcard matches
desc += 20
continue
dname = raw_dname.decode("ascii", "replace").upper()
if tdll is None or dname == tdll:
thunk = oft or ft # fall back to FT for bound imports
i = 0
```
</issue_to_address>
### Comment 2
<location path="scripts/jvm_boot_glue.py" line_range="1059-1068" />
<code_context>
+def _augment_dll_search_path(jdk_bin: Path) -> list[object]:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** DLL directory cookies are scoped to launch_minecraft; future refactors could accidentally shorten their lifetime.
The docstring says AddDllDirectory cookies must live for the lifetime of the JVM, but they’re only stored in the local `_dll_search_cookies` inside `launch_minecraft`, making that guarantee implicit in the current control flow. To make this resilient to refactors, please store the cookies on a longer‑lived object (e.g., as an attribute on `JvmBootGlue`, the loader, or a module‑level list) so their lifetime is explicitly tied to the JVM/process rather than the function scope.
Suggested implementation:
```python
def _augment_dll_search_path(
jdk_bin: Path,
*,
cookie_sink: list[object] | None = None,
) -> list[object]:
"""
Make ``jdk_bin`` and ``jdk_bin/server`` discoverable by the OS loader for
the lifetime of the returned cookies.
``cookie_sink``, if provided, is extended with the created AddDllDirectory
cookies so their lifetime is tied to that longer-lived container (for
example an attribute on ``JvmBootGlue``) rather than to a local variable in
the caller.
Even in memory-resident mode, several auxiliary JDK libraries (jawt.dll,
sunmscapi.dll, awt.dll, …) are loaded by the JVM through the regular OS
loader after JNI_CreateJavaVM completes. AddDllDirectory provides a
narrowly-scoped search list without polluting PATH.
Returns
```
To fully implement the longer-lived lifetime for the AddDllDirectory cookies, you’ll also need to:
1. Inside `_augment_dll_search_path`, after you build the list of cookies (whatever variable currently holds them, e.g. `cookies` or `_dll_search_cookies`), add:
```python
if cookie_sink is not None:
cookie_sink.extend(cookies)
```
immediately before the `return cookies` statement.
2. In the code that calls `_augment_dll_search_path` (likely inside `launch_minecraft`):
- Replace the current local-only storage:
```python
_dll_search_cookies = _augment_dll_search_path(jdk_bin)
```
with something that uses a longer-lived container, for example on `JvmBootGlue`:
```python
if not hasattr(self, "_dll_search_cookies"):
self._dll_search_cookies: list[object] = []
_augment_dll_search_path(jdk_bin, cookie_sink=self._dll_search_cookies)
```
or, if `launch_minecraft` is not a method, use a module-level list instead:
```python
_DLL_SEARCH_COOKIES: list[object] = []
...
_augment_dll_search_path(jdk_bin, cookie_sink=_DLL_SEARCH_COOKIES)
```
3. Remove any now-unused local `_dll_search_cookies` variables so that the only strong references to the cookies are on the longer-lived object (e.g. `self._dll_search_cookies` or the module-level `_DLL_SEARCH_COOKIES`).
These changes ensure the AddDllDirectory cookies are explicitly tied to the JVM/process lifetime via a long-lived container, making the guarantee resilient to future refactors of `launch_minecraft`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+480
to
483
| dname = _safe_cstr(module_base + nrv).decode("ascii", "replace").upper() | ||
| if tdll is None or dname == tdll: | ||
| thunk = oft or ft # fall back to FT for bound imports | ||
| i = 0 |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Empty strings from _safe_cstr on unreadable memory are treated as valid DLL names when target_dll is None.
Because _safe_cstr returns b"" on unreadable memory, you end up treating an empty DLL name as a match when target_dll is None and then walking its thunk table. For malformed images this adds unnecessary work, can surface extra OSErrors, and makes the matching semantics less clear. Consider skipping descriptors with empty names (e.g. if not dname: desc += 20; continue) so only readable, non-empty names are included in the wildcard search.
Suggested change
| dname = _safe_cstr(module_base + nrv).decode("ascii", "replace").upper() | |
| if tdll is None or dname == tdll: | |
| thunk = oft or ft # fall back to FT for bound imports | |
| i = 0 | |
| raw_dname = _safe_cstr(module_base + nrv) | |
| if not raw_dname: | |
| # Skip unreadable or empty DLL names when doing wildcard matches | |
| desc += 20 | |
| continue | |
| dname = raw_dname.decode("ascii", "replace").upper() | |
| if tdll is None or dname == tdll: | |
| thunk = oft or ft # fall back to FT for bound imports | |
| i = 0 |
Comment on lines
+1059
to
+1068
| def _augment_dll_search_path(jdk_bin: Path) -> list[object]: | ||
| """ | ||
| Make ``jdk_bin`` and ``jdk_bin/server`` discoverable by the OS loader for | ||
| the lifetime of the returned cookies. | ||
|
|
||
| Even in memory-resident mode, several auxiliary JDK libraries (jawt.dll, | ||
| sunmscapi.dll, awt.dll, …) are loaded by the JVM through the regular OS | ||
| loader after JNI_CreateJavaVM completes. AddDllDirectory provides a | ||
| narrowly-scoped search list without polluting PATH. | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): DLL directory cookies are scoped to launch_minecraft; future refactors could accidentally shorten their lifetime.
The docstring says AddDllDirectory cookies must live for the lifetime of the JVM, but they’re only stored in the local _dll_search_cookies inside launch_minecraft, making that guarantee implicit in the current control flow. To make this resilient to refactors, please store the cookies on a longer‑lived object (e.g., as an attribute on JvmBootGlue, the loader, or a module‑level list) so their lifetime is explicitly tied to the JVM/process rather than the function scope.
Suggested implementation:
def _augment_dll_search_path(
jdk_bin: Path,
*,
cookie_sink: list[object] | None = None,
) -> list[object]:
"""
Make ``jdk_bin`` and ``jdk_bin/server`` discoverable by the OS loader for
the lifetime of the returned cookies.
``cookie_sink``, if provided, is extended with the created AddDllDirectory
cookies so their lifetime is tied to that longer-lived container (for
example an attribute on ``JvmBootGlue``) rather than to a local variable in
the caller.
Even in memory-resident mode, several auxiliary JDK libraries (jawt.dll,
sunmscapi.dll, awt.dll, …) are loaded by the JVM through the regular OS
loader after JNI_CreateJavaVM completes. AddDllDirectory provides a
narrowly-scoped search list without polluting PATH.
ReturnsTo fully implement the longer-lived lifetime for the AddDllDirectory cookies, you’ll also need to:
-
Inside
_augment_dll_search_path, after you build the list of cookies (whatever variable currently holds them, e.g.cookiesor_dll_search_cookies), add:if cookie_sink is not None: cookie_sink.extend(cookies)
immediately before the
return cookiesstatement. -
In the code that calls
_augment_dll_search_path(likely insidelaunch_minecraft):- Replace the current local-only storage:
with something that uses a longer-lived container, for example on
_dll_search_cookies = _augment_dll_search_path(jdk_bin)
JvmBootGlue:or, ifif not hasattr(self, "_dll_search_cookies"): self._dll_search_cookies: list[object] = [] _augment_dll_search_path(jdk_bin, cookie_sink=self._dll_search_cookies)
launch_minecraftis not a method, use a module-level list instead:_DLL_SEARCH_COOKIES: list[object] = [] ... _augment_dll_search_path(jdk_bin, cookie_sink=_DLL_SEARCH_COOKIES)
- Replace the current local-only storage:
-
Remove any now-unused local
_dll_search_cookiesvariables so that the only strong references to the cookies are on the longer-lived object (e.g.self._dll_search_cookiesor the module-level_DLL_SEARCH_COOKIES).
These changes ensure the AddDllDirectory cookies are explicitly tied to the JVM/process lifetime via a long-lived container, making the guarantee resilient to future refactors of launch_minecraft.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Fix Windows PE import table parsing and JVM module-handle handling for memory-resident JVM boot, and harden JVM path and environment setup when launching Minecraft.
Bug Fixes:
Enhancements: