[WebAssembly] Fix stack overflow in crc32_chorba_118960_nondestructive.#1915
[WebAssembly] Fix stack overflow in crc32_chorba_118960_nondestructive.#1915Dead2 merged 1 commit intozlib-ng:developfrom
Conversation
WalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant crc32_chorba_118960_nondestructive
participant zng_alloc
participant zng_free
Caller->>crc32_chorba_118960_nondestructive: call
alt Compiling for Emscripten
crc32_chorba_118960_nondestructive->>zng_alloc: allocate bitbuffer
zng_alloc-->>crc32_chorba_118960_nondestructive: pointer to buffer
Note over crc32_chorba_118960_nondestructive: Use buffer for computation
crc32_chorba_118960_nondestructive->>zng_free: free bitbuffer
else Not Emscripten
Note over crc32_chorba_118960_nondestructive: Use stack-allocated bitbuffer
end
crc32_chorba_118960_nondestructive-->>Caller: return
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
arch/generic/crc32_chorba_c.c (1)
31-492: Consider error handling for memory allocation failure.The current implementation doesn't check if
zng_alloc()successfully allocated memory. For a more robust solution, consider adding error handling for allocation failures.#if defined(__EMSCRIPTEN__) z_word_t* bitbuffer = (z_word_t*)zng_alloc(bitbuffersizebytes); + if (bitbuffer == NULL) { + /* Handle allocation failure - options include: + * 1. Return an error code + * 2. Fallback to a slower algorithm that uses less memory + * 3. Assert/abort if this is a critical function that cannot fail + */ + return 0; /* Or appropriate error value */ + } #else ALIGNED_(16) z_word_t bitbuffer[bitbuffersizezwords]; #endifAnd update the cleanup section accordingly:
#if defined(__EMSCRIPTEN__) + if (bitbuffer != NULL) { zng_free(bitbuffer); + } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
arch/generic/crc32_chorba_c.c(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (68)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
- GitHub Check: Ubuntu GCC PPC
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2022 v143 Win32
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC SSSE3 UBSAN
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: Ubuntu GCC S390X No vectorized CRC32
🔇 Additional comments (4)
arch/generic/crc32_chorba_c.c (4)
2-4: Good addition of conditional include for Emscripten.Including
zutil_p.honly for Emscripten builds is a clean approach that avoids unnecessary dependencies in other build environments.
31-35: Effective solution to the stack overflow issue.The change to use dynamic memory allocation in Emscripten addresses the root cause of the stack overflow. The default Emscripten stack size (64 KB) is insufficient for the 128 KB temporary buffer, as noted in the function's comment on line 28.
This is a good approach as it:
- Only affects the Emscripten target
- Preserves the aligned stack allocation for other platforms where it's efficient
- Avoids modifying the algorithm's logic
490-492: Proper resource cleanup.The addition of
zng_free()correctly prevents memory leaks by freeing the dynamically allocated buffer in the Emscripten-specific code path.
10-11: Verify the buffer size requirement across platforms.The bitbuffer size is defined as 16 * 1024 * sizeof(z_word_t), which is 128 KB on platforms where z_word_t is 8 bytes. While the PR fixes the Emscripten issue, it's worth noting that this function requires a large temporary buffer as documented in the function comments.
This is for information only - no changes needed to the implementation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1915 +/- ##
===========================================
- Coverage 83.42% 81.65% -1.78%
===========================================
Files 144 147 +3
Lines 12948 13308 +360
Branches 2857 2951 +94
===========================================
+ Hits 10802 10866 +64
- Misses 1202 1516 +314
+ Partials 944 926 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Emscripten doesn't by default need NULL check for allocation failure as it will abort on out-of-memory condition. |
Summary by CodeRabbit