Skip to content

Fix null-termination and resource leaks in asset loading#19

Merged
MisterGatto merged 1 commit intoskyprotocol:osp-masterfrom
HugeFrog24:osp-master
Mar 28, 2026
Merged

Fix null-termination and resource leaks in asset loading#19
MisterGatto merged 1 commit intoskyprotocol:osp-masterfrom
HugeFrog24:osp-master

Conversation

@HugeFrog24
Copy link
Copy Markdown
Contributor

@HugeFrog24 HugeFrog24 commented Mar 28, 2026

Context

Hey I think we hit a crash loop under houdini (ARM-on-x86 emulators) caused by readAsset returning a char* with no null terminator. json::parse calls strlen on it, which walks past the allocation into unmapped memory. On native ARM64, this never surfaces because adjacent heap bytes tend to be zeroed by luck. On houdini the memory layout is different and it faults immediately.

Any mod using read_asset as a C string has the same latent UB.

Proposed fix

  • Let CipherUtils::readAsset() null-terminate the returned buffer (malloc +1, write '\0')
  • Fixed AAsset handle leaks on malloc/read failure paths in both readAsset and uploadImageKtx
  • Fixed dangling stack reference in IconLoader::die() (local → static)
  • Fixed missing buffer/texture cleanup on error paths in uploadImageKtx

Testing

  • Load a mod that calls Cipher::read_asset() -> verify asset data is intact (no truncation, no corruption)
  • Trigger malloc pressure if possible -> verify no crashes on the error paths (previously leaked AAsset handles)
  • Load custom icons via IconLoader -> verify KTX textures still render correctly
  • Test on both native ARM64 device and houdini emulator (e.g. MuMu) if available

Open to discussion and happy to adjust if needed.

CipherUtils::readAsset(): allocate +1 for null terminator, close
AAsset on malloc and read failure paths.

IconLoader::uploadImageKtx(): fix dangling stack reference in die()
by using a static sentinel, close AAsset and free buffer on all
error paths, add missing return on glGenTextures failure.
@HugeFrog24
Copy link
Copy Markdown
Contributor Author

CI failed during release packaging due to a keystore password mismatch. Pre-existing issue, unrelated to this commit.

@MisterGatto
Copy link
Copy Markdown
Collaborator

MuMu specifically needs a different package name which is com.tgc.sky.android.sml to not crash, Ionly know of Bluestacks that works with no issue with normal name, does this only happens with your mod? Because if it does and the fix you proposed is working without giving any issues for Android users then we can squash and merge, I'm currentyl unable to make a debug build because I can't use my PC, if you can send the apk via telegram or another platform I will begin testing in Canvas cord.

@MisterGatto MisterGatto merged commit 6bd2ef2 into skyprotocol:osp-master Mar 28, 2026
1 check failed
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