fix: add missing zend_string_release calls to prevent memory leaks#77
Conversation
WalkthroughAdds zend_string_release(dict) on two error paths in php_brotli_output_handler_load_dict within brotli.c to free the allocated dictionary string before setting it to NULL. Other control flow and existing frees remain unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
brotli.c (1)
473-485: Memory leak: dict not released in early-return and success paths
php_brotli_output_handler_load_dict()returns a freshly allocated zend_string that must be released by the caller. Inphp_brotli_output_handler_context_start():
- If
BROTLI_G(compression_coding)is false (Lines 475-477), we return without releasingdict→ leak.- On the success (or failure) path of
php_brotli_context_create_encoder_ex(...)(Lines 479-485),dictis not released. Per team learning, ownership is not transferred to the encoder create function; cleanup is handled by callers (see retrieved learnings).Proposed fix releases
dictin all return paths:static int php_brotli_output_handler_context_start(php_brotli_context *ctx) { long level = BROTLI_G(output_compression_level); zend_string *dict = php_brotli_output_handler_load_dict(ctx); if (!BROTLI_G(compression_coding)) { - return FAILURE; + if (dict) { + zend_string_release(dict); + } + return FAILURE; } - return php_brotli_context_create_encoder_ex(ctx, - level, - BROTLI_DEFAULT_WINDOW, - BROTLI_MODE_GENERIC, - dict, - 0); + int rc = php_brotli_context_create_encoder_ex(ctx, + level, + BROTLI_DEFAULT_WINDOW, + BROTLI_MODE_GENERIC, + dict, + 0); + if (dict) { + zend_string_release(dict); + } + return rc; }
🧹 Nitpick comments (1)
brotli.c (1)
452-452: Consistency nit: prefer zend_string_release over zend_string_freeElsewhere we use
zend_string_release(dict). For consistency and to avoid confusion about refcount semantics, consider usingzend_string_release(b64)here as well.- zend_string_free(b64); + zend_string_release(b64);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
brotli.c(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-02T04:03:47.808Z
Learnt from: kjdev
PR: kjdev/php-ext-brotli#64
File: brotli.c:122-134
Timestamp: 2025-05-02T04:03:47.808Z
Learning: In the php-ext-brotli extension, encoder/decoder resource cleanup is deliberately handled by the caller functions rather than within resource creation functions like `php_brotli_context_create_encoder_ex`. The cleanup is done via `php_brotli_context_close()` which properly destroys any encoder/decoder instances.
Applied to files:
brotli.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: ci (8.1, x86, nts, vs16, false)
- GitHub Check: ci (8.1, x86, nts, vs16, true)
- GitHub Check: ci (8.1, x86, ts, vs16, true)
- GitHub Check: ci (8.1, x64, nts, vs16, true)
- GitHub Check: ci (8.1, x86, ts, vs16, false)
- GitHub Check: ci (8.1-zts-alpine, system, apcu, php, linux/amd64)
- GitHub Check: ci (8.2, x86, ts, vs16, false)
- GitHub Check: ci (8.2, x86, nts, vs16, true)
- GitHub Check: ci (8.3, x86, ts, vs16, true)
- GitHub Check: ci (8.3, x64, ts, vs16, true)
- GitHub Check: ci (8.2, x64, nts, vs16, false)
- GitHub Check: ci (8.3, x86, nts, vs16, true)
- GitHub Check: ci (8.3, x86, nts, vs16, false)
- GitHub Check: ci (8.3, x64, ts, vs16, false)
🔇 Additional comments (2)
brotli.c (2)
449-451: Good catch: release dict on invalid available-dictionary pathReleasing the zend_string before nulling it fixes a real leak on this error path. Looks correct and safe.
458-460: Good catch: release dict when Available-Dictionary header is missingSame here—this closes the leak when the header isn’t present. Change looks good.
1 participant
Summary by CodeRabbit