Change/incorrect parameters cause errors#64
Conversation
WalkthroughThis update introduces stricter validation and enhanced error handling for Brotli compression and decompression in the PHP extension. The encoder creation function is refactored to accept an additional parameter controlling error behavior, with improved error messages and conditional fallbacks. Validation for compression quality and mode is now stricter, aborting operations on invalid input. Incremental compression and decompression functions also validate operation modes, emitting warnings and returning failure on invalid values. Corresponding tests are added and updated to verify argument validation and error handling, including cases with invalid parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PHP_Extension
participant Brotli_Encoder
User->>PHP_Extension: brotli_compress(data, quality, mode)
PHP_Extension->>PHP_Extension: Validate quality and mode
alt Invalid parameters
PHP_Extension-->>User: Emit warning, return failure
else Valid parameters
PHP_Extension->>Brotli_Encoder: Create encoder with params
alt Encoder creation fails
PHP_Extension-->>User: Emit error, return failure
else Encoder created
PHP_Extension->>Brotli_Encoder: Compress data
Brotli_Encoder-->>PHP_Extension: Compressed data
PHP_Extension-->>User: Return compressed data
end
end
sequenceDiagram
participant User
participant PHP_Extension
User->>PHP_Extension: brotli_compress_add(resource, data, mode)
PHP_Extension->>PHP_Extension: Validate mode
alt Invalid mode
PHP_Extension-->>User: Emit warning, return false
else Valid mode
PHP_Extension->>PHP_Extension: Compress incrementally
PHP_Extension-->>User: Return result
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (6)
✨ Finishing Touches
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 (
|
|
@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: 2
🔭 Outside diff range comments (2)
brotli.c (2)
138-153:⚠️ Potential issueSymmetric leak on invalid
modepathThe same leak scenario exists for an invalid
mode(§ lines 138-153). The encoder handle must be destroyed before returning.@@ - if (fail) { - return FAILURE; - } + if (fail) { + BrotliEncoderDestroyInstance(ctx->encoder); + ctx->encoder = NULL; + return FAILURE; + }
135-137: 🛠️ Refactor suggestionNo range validation for
lgwinparameter
lgwindefaults toBROTLI_DEFAULT_WINDOW, but if the caller passes an explicit (non-zero) value outside Brotli’s supported 10-24 range the subsequentBrotliEncoderSetParametercall will just fail, causing the generic “failed to set compression parameters” message later.
Consider performing an explicit bounds check similar toqualityandmode, so users receive precise feedback.
🧹 Nitpick comments (3)
brotli.c (1)
155-164: Minor: chain ofBrotliEncoderSetParametercalls obscures which parameter failedUsing the logical OR chain loses information about which individual
SetParameterfailed. Splitting them or at least checking the return value one-by-one would surface the actual reason to users and aid debugging.tests/compress_args.phpt (1)
23-28: Tiny nit — use constants for readabilityInstead of raw
0,1,2literals formode, consider the public constants (BROTLI_GENERIC,BROTLI_TEXT,BROTLI_FONT). This makes the intent clearer and keeps the test resilient to future enum changes. Same remark forqualitywhere you could reuseBROTLI_COMPRESS_LEVEL_*macros.tests/incremental_compress_add_args.phpt (1)
45-51: Nit — duplicate loop valuesThe second
foreachuses[0, 1, 2, 3, -1], but the first value (0) duplicates the previous quality loop and produces identical output. Trimming unnecessary iterations can shorten test run-time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
brotli.c(6 hunks)tests/compress_args.phpt(1 hunks)tests/incremental_compress_add_args.phpt(4 hunks)
| if (quality < BROTLI_MIN_QUALITY || quality > BROTLI_MAX_QUALITY) { | ||
| php_error_docref(NULL, E_WARNING, | ||
| "brotli: compression level (%ld) " | ||
| "must be within %d..%d", | ||
| (long)quality, BROTLI_MIN_QUALITY, BROTLI_MAX_QUALITY); | ||
| "%s compression quality (%ld)%s:" | ||
| " must be within %d..%d", | ||
| (fail ? "failed to" : "brotli: set"), | ||
| (long)quality, | ||
| (fail ? "": " to BROTLI_COMPRESS_LEVEL_DEFAULT"), | ||
| BROTLI_MIN_QUALITY, BROTLI_MAX_QUALITY); | ||
| if (fail) { | ||
| return FAILURE; | ||
| } | ||
| quality = BROTLI_DEFAULT_QUALITY; | ||
| } |
There was a problem hiding this comment.
Possible memory leak when quality validation fails
BrotliEncoderCreateInstance() is called at line 115 and the returned handle is stored in ctx->encoder.
If quality is out of range and fail is non-zero, control jumps to this return FAILURE; path, but the previously-allocated encoder instance is never destroyed, leaking native memory.
@@
- if (fail) {
- return FAILURE;
- }
+ if (fail) {
+ BrotliEncoderDestroyInstance(ctx->encoder);
+ ctx->encoder = NULL;
+ return FAILURE;
+ }Please apply similar cleanup wherever early-returns occur after the encoder has been created.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (quality < BROTLI_MIN_QUALITY || quality > BROTLI_MAX_QUALITY) { | |
| php_error_docref(NULL, E_WARNING, | |
| "brotli: compression level (%ld) " | |
| "must be within %d..%d", | |
| (long)quality, BROTLI_MIN_QUALITY, BROTLI_MAX_QUALITY); | |
| "%s compression quality (%ld)%s:" | |
| " must be within %d..%d", | |
| (fail ? "failed to" : "brotli: set"), | |
| (long)quality, | |
| (fail ? "": " to BROTLI_COMPRESS_LEVEL_DEFAULT"), | |
| BROTLI_MIN_QUALITY, BROTLI_MAX_QUALITY); | |
| if (fail) { | |
| return FAILURE; | |
| } | |
| quality = BROTLI_DEFAULT_QUALITY; | |
| } | |
| if (quality < BROTLI_MIN_QUALITY || quality > BROTLI_MAX_QUALITY) { | |
| php_error_docref(NULL, E_WARNING, | |
| "%s compression quality (%ld)%s:" | |
| " must be within %d..%d", | |
| (fail ? "failed to" : "brotli: set"), | |
| (long)quality, | |
| (fail ? "" : " to BROTLI_COMPRESS_LEVEL_DEFAULT"), | |
| BROTLI_MIN_QUALITY, BROTLI_MAX_QUALITY); | |
| if (fail) { | |
| BrotliEncoderDestroyInstance(ctx->encoder); | |
| ctx->encoder = NULL; | |
| return FAILURE; | |
| } | |
| quality = BROTLI_DEFAULT_QUALITY; | |
| } |
There was a problem hiding this comment.
release processing is now performed by the caller.
There was a problem hiding this comment.
@kjdev Thanks for the clarification. I see now that the calling functions (like brotli_compress and others) are handling the cleanup by calling php_brotli_context_close(&ctx) when an error occurs. This design properly manages memory resources without leaking, as the responsibility for cleanup is consistently placed with the caller rather than within the encoder creation function.
✏️ Learnings added
Learnt from: kjdev
PR: kjdev/php-ext-brotli#64
File: brotli.c:122-134
Timestamp: 2025-05-02T04:03:47.781Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
kjdev
force-pushed
the
change/incorrect-parameters-cause-errors
branch
from
174a23b to
5d461de
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
Bug Fixes
Tests