Skip to content

fix: encryption system crashes and data corruption when ENABLE_ENCRYPTION=1#1684

Open
Kizuno18 wants to merge 1 commit intoopentibiabr:mainfrom
Kizuno18:fix/encryption-crashes
Open

fix: encryption system crashes and data corruption when ENABLE_ENCRYPTION=1#1684
Kizuno18 wants to merge 1 commit intoopentibiabr:mainfrom
Kizuno18:fix/encryption-crashes

Conversation

@Kizuno18
Copy link
Copy Markdown
Contributor

@Kizuno18 Kizuno18 commented Apr 4, 2026

Summary

  • readFileContents() crashes on startup by accessing g_game.getFeature() before g_game is constructed — removed the dependency; non-encrypted files now pass through instead of returning empty string
  • writeFileContents() encrypts ALL writes (including user configs), corrupting them on restart — removed runtime encryption; only --encrypt builder should encrypt
  • FileStream::cache() decrypts unconditionally (even without header) and uses std::memcmp instead of std::memcpy — now only decrypts when header is present, using in-place decrypt
  • logger.cpp silences all logs when encryption is enabled — removed the conditional

Test plan

  • Build with ENABLE_ENCRYPTION=0 — no behavior change
  • Build with ENABLE_ENCRYPTION=1 + ENABLE_ENCRYPTION_BUILDER=1, run --encrypt on a copy of data/, modules/, mods/, init.lua
  • Rebuild with ENABLE_ENCRYPTION=1 + ENABLE_ENCRYPTION_BUILDER=0 (final exe)
  • Run from encrypted assets — client loads and runs normally
  • Run from non-encrypted assets — client loads and runs normally (passthrough)
  • Verify user configs (config.otml) are saved as plaintext and survive restart

…TION=1

Three bugs in the encryption system that make ENABLE_ENCRYPTION=1 unusable:

1. readFileContents() crashes on startup — accesses g_game.getFeature()
   which runs during g_resources.init(), before g_game is constructed.
   Removed the g_game dependency and the bot script feature gate (unrelated
   to encryption). Non-encrypted files now pass through instead of returning
   empty string, which also broke reading configs, .dat, .spr, etc.

2. writeFileContents() encrypts ALL file writes including user configs
   (config.otml, hotkeys, etc.), corrupting them on subsequent reads when
   the client restarts without encryption. Only the --encrypt builder
   should encrypt; runtime writes must remain plaintext.

3. FileStream::cache() decrypts unconditionally even when the file has no
   encryption header, causing garbage output. Also used std::memcmp instead
   of std::memcpy to copy decrypted data (compares but never copies), and
   resized the buffer incorrectly when no header was present.

4. logger.cpp silences ALL log output when ENABLE_ENCRYPTION=1, making
   debugging impossible. Removed the conditional — logging should work
   regardless of encryption state.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

📝 Walkthrough

Walkthrough

This PR simplifies encryption handling across the framework core by refactoring the FileStream::cache() decryption logic to use a cleaner API, decoupling logging behavior from encryption settings, and removing encryption logic from ResourceManager::writeFileContents() while streamlining header detection in readFileContents().

Changes

Cohort / File(s) Summary
FileStream encryption refactoring
src/framework/core/filestream.cpp
Simplified cache() method's encryption header detection and decryption by replacing manual temporary buffer allocation/resize with direct ResourceManager::decrypt() call on stripped cached data.
Logger and ResourceManager encryption decoupling
src/framework/core/logger.cpp, src/framework/core/resourcemanager.cpp
Removed conditional ENABLE_ENCRYPTION checks: logger's s_ignoreLogs now always initializes to false, and removed conditional client/game.h include from ResourceManager. Simplified encryption-header check in readFileContents() and removed all encryption logic from writeFileContents().

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 Encryption's chains we gently loosen,
No more headers tangled in confusion,
Logging flows free from cipher's hold,
Simpler paths, more stories told! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main objective of the changeset—fixing encryption-related crashes and data corruption when the ENABLE_ENCRYPTION flag is enabled.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant