Skip to content

Support 64-bit sizes in Compression #106190

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 1 commit into from
Jun 3, 2025

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 8, 2025

I was working on implementing (de)compressing some data, and I noticed that Godot's Compression class uses 32-bit int for sizes. This is not good, it means that you can't use this class to (de)compress data larger than 2 GiB.

This PR upgrades these functions to use int64_t instead. Specifically, here are some notes about this class:

  • This must be a signed integer because compression failure returns -1 and auto buffer size parameter is -1.
  • FastLZ, Deflate, and GZip compression are limited to 2 GiB because int is used in those libraries. Furthermore, the underlying formats of Deflate and GZip are limited to 4 GiB. For all of these formats, I have added ERR_FAIL_COND_V_MSG macros that check for this and explain the problem.
  • Zstd, and decompressing Brotli, are the only compression formats we have that support 64-bit sizes, they use size_t, but since Godot uses int64_t they are limited to sizes under 2^63.

I also looked around the codebase for places where these functions have been used.

  • In many places, upgrade the type used to store the return value to int64_t.
    • In most cases this is just compared == -1 so there are no side effect changes in most cases.
  • In many places, add explicit casts to make it clear when a cast is happening.
  • In FileAccessCompressed, it was not checking if the compression succeeded (returns -1 if failed). I added ERR_FAIL_COND_MSG to check this.
  • In EditorFileServer and CryptoMbedTLS, these classes were not checking if the decompression succeeded. Furthermore, the classes were just making the assumption that the decompressed size matched the expected size. If it did not match, the code in master could result in undefined behavior reading from uninitialized memory. I added ERR_FAIL_COND_MSG checks to require that the actual decompressed size matches the expected size.
  • In RenderingDeviceDriverVulkan/D3D12, those classes are using uint32_t for storing sizes, so this change from int to int64_t upgrades the limit from 2 GiB to 4 GiB if using a compression format that supports more than 2 GiB.
  • In DocTools, upgrade those functions to 64-bit as well.

I have not extensively tested this PR, this should be done before merging.

Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

This looks good to me! The only caveat I can think of is that possibly this breaks compatibility with modules using these interfaces.

What we need before merging is an example of one or two files that were incorrectly handled before, whose handling is improved with this PR.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, I can't get it to work.

Using resize() beyond 2 GB on a PackedByteArray will still result in a crash after this PR:

handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.5.dev.custom_build (d6a0f3856c0f06c807a983dc7c20f2d6df9cb242)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[1] /lib64/libc.so.6(+0x19c30) [0x7f6a226b5c30] (??:0)
[2] /lib64/libc.so.6(+0x15f7fc) [0x7f6a227fb7fc] (??:0)
[3] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xaee4cab] (/home/hugo/Documents/Git/godotengine/godot/core/io/file_access_compressed.cpp:329)
[4] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xaeddba1] (/home/hugo/Documents/Git/godotengine/godot/core/io/file_access.cpp:812)
[5] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xaf0d54b] (/home/hugo/Documents/Git/godotengine/godot/core/variant/binder_common.h:672 (discriminator 6))
[6] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xaf03705] (/home/hugo/Documents/Git/godotengine/godot/core/variant/binder_common.h:455)
[7] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xaef85db] (/home/hugo/Documents/Git/godotengine/godot/core/object/method_bind.h:515 (discriminator 1))
[8] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xb68643c] (/home/hugo/Documents/Git/godotengine/godot/core/object/object.cpp:872 (discriminator 1))
[9] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xb18c77b] (/home/hugo/Documents/Git/godotengine/godot/core/variant/variant_call.cpp:1316 (discriminator 2))
[10] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x4dbe44d] (/home/hugo/Documents/Git/godotengine/godot/modules/gdscript/gdscript.h:595)
[11] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x4b70391] (/home/hugo/Documents/Git/godotengine/godot/modules/gdscript/gdscript.cpp:2058 (discriminator 1))
[12] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x80905e6] (/home/hugo/Documents/Git/godotengine/godot/core/variant/variant.h:868)
[13] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x427ee7b] (/home/hugo/Documents/Git/godotengine/godot/scene/main/node.h:48)
[14] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x427f983] (/home/hugo/Documents/Git/godotengine/godot/scene/main/canvas_item.h:43)
[15] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x42810d5] (/home/hugo/Documents/Git/godotengine/godot/scene/2d/node_2d.h:36)
[16] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0xb68681f] (/home/hugo/Documents/Git/godotengine/godot/core/object/object.cpp:933)
[17] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x8090fb9] (/home/hugo/Documents/Git/godotengine/godot/core/object/object.h:881)
[18] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x8090ed7] (/home/hugo/Documents/Git/godotengine/godot/core/templates/hash_map.h:481)
[19] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x80ab0a0] (/home/hugo/Documents/Git/godotengine/godot/scene/main/node.cpp:3450)
[20] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x816befb] (/home/hugo/Documents/Git/godotengine/godot/scene/main/scene_tree.cpp:566)
[21] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x3888971] (/home/hugo/Documents/Git/godotengine/godot/servers/display_server.h:60)
[22] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x387c631] (/home/hugo/Documents/Git/godotengine/godot/platform/linuxbsd/godot_linuxbsd.cpp:85)
[23] /lib64/libc.so.6(+0x35f5) [0x7f6a2269f5f5] (??:0)
[24] /lib64/libc.so.6(__libc_start_main+0x88) [0x7f6a2269f6a8] (??:0)
[25] /home/hugo/Documents/Git/godotengine/godot/bin/godot.linuxbsd.editor.x86_64() [0x387c425] (??:?)
-- END OF C++ BACKTRACE --

I've also tried another approach like using store_string() many times, but it will also crash when the FileAccessCompressed exceeds 2 GB.

Testing project: test_pr_106190.zip

@aaronfranke
Copy link
Member Author

@Calinou The problem in your project is specifically with FileAccessCompressed and the next_power_of_2 function it calls, the Compression class itself works with just this PR. If I upgrade next_power_of_2 to 64-bit and also change uint32_t write_buffer_size then your test project works. I opened another PR for this: #106606

@hpvb
Copy link
Member

hpvb commented May 28, 2025

Approved pending testing of this together with the power of 2 changes.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

The testing project I linked above works when combining this PR with #106606. Code looks good to me. Great work 🙂

@akien-mga akien-mga modified the milestones: 4.x, 4.5 Jun 3, 2025
@Repiteo Repiteo merged commit 344b8ce into godotengine:master Jun 3, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 3, 2025

Thanks!

@aaronfranke aaronfranke deleted the compression64 branch June 3, 2025 20:53
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.

6 participants