added #2
added #2
Conversation
WalkthroughA new source file introduces a blacklist mechanism for Zend OPcache, enabling exclusion of specific paths from caching via user-defined patterns. It manages entries, compiles them into PCRE2 regexes, loads patterns from files, normalizes paths, supports matching logic, and provides initialization, shutdown, and utility functions for external processing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Blacklist
participant FileSystem
participant PCRE2
User->>Blacklist: Initialize (zend_accel_blacklist_init)
User->>Blacklist: Load patterns (zend_accel_blacklist_load)
Blacklist->>FileSystem: Read blacklist file(s)
Blacklist->>PCRE2: Compile patterns into regex
User->>Blacklist: Check path (zend_accel_blacklist_is_blacklisted)
Blacklist->>PCRE2: Match path against regex
PCRE2-->>Blacklist: Return match result
Blacklist-->>User: Return blacklist status
User->>Blacklist: Shutdown (zend_accel_blacklist_shutdown)
Blacklist->>PCRE2: Free regex resources
Poem
✨ Finishing Touches
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
opcache/zend_accelerator_blacklist.c (4)
62-65: Remove unreachable code after fatal error.The
returnstatement afterzend_accel_error_noreturn(ACCEL_LOG_FATAL, ...)is unreachable since fatal errors should not return. This could indicate a logical error or dead code.if (!blacklist->entries) { zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Blacklist initialization: no memory\n"); - return; }
80-80: Consider dynamic buffer allocation for large patterns.The fixed 12KB buffer size may be insufficient for very long blacklist patterns or numerous entries. Consider using dynamic allocation or increasing the buffer size.
- char regexp[12*1024], *p, *end, *c, *backtrack = NULL; + char *regexp = malloc(32*1024); // Or use dynamic sizing + if (!regexp) { + zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Failed to allocate regex buffer\n"); + return; + } + char *p, *end, *c, *backtrack = NULL;
360-363: Consider logging allocation failures.When
match_dataallocation fails, the function silently continues to the next pattern. This could mask serious memory issues and cause inconsistent blacklist behavior.if (!match_data) { /* Alloc failed, but next one could still come through and match. */ + zend_accel_error(ACCEL_LOG_WARNING, "Failed to allocate match data for blacklist check\n"); continue; }
1-384: Overall implementation assessment.The blacklist implementation provides comprehensive functionality with good architectural design. However, there are several critical issues that need to be addressed:
- Memory safety: Missing NULL checks after allocation calls
- Buffer overflow protection: Fixed-size buffers may be insufficient
- Logic errors: Incorrect error handling in backtracking logic
- Thread safety: Consider thread safety implications of global variables
The core functionality is sound, but the implementation needs hardening for production use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
opcache/zend_accelerator_blacklist.c(1 hunks)
🔇 Additional comments (2)
opcache/zend_accelerator_blacklist.c (2)
208-230: Well-implemented cleanup function.The shutdown function properly handles NULL checks and frees all allocated resources in the correct order. Good defensive programming.
164-171: Fix logical error in backtracking logic.When
backtrackis NULL and the pattern is too long, the function logs an error but continues execution, which could lead to undefined behavior. The function should return early after logging the error.if (*c) { if (!backtrack) { zend_accel_error_noreturn(ACCEL_LOG_ERROR, "Too long blacklist entry\n"); + return; } p = backtrack; } else { i++; }Likely an incorrect or invalid review comment.
| if (blacklist->pos == blacklist->size) { | ||
| blacklist->size += ZEND_BLACKLIST_BLOCK_SIZE; | ||
| blacklist->entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry)*blacklist->size); | ||
| } |
There was a problem hiding this comment.
Add NULL check after realloc to prevent memory leak.
If realloc fails, it returns NULL but doesn't free the original memory. The current code would lose the original pointer, causing a memory leak and potential crash.
if (blacklist->pos == blacklist->size) {
blacklist->size += ZEND_BLACKLIST_BLOCK_SIZE;
- blacklist->entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry)*blacklist->size);
+ zend_blacklist_entry *new_entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry)*blacklist->size);
+ if (!new_entries) {
+ zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Failed to reallocate blacklist entries\n");
+ return;
+ }
+ blacklist->entries = new_entries;
}📝 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 (blacklist->pos == blacklist->size) { | |
| blacklist->size += ZEND_BLACKLIST_BLOCK_SIZE; | |
| blacklist->entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry)*blacklist->size); | |
| } | |
| if (blacklist->pos == blacklist->size) { | |
| blacklist->size += ZEND_BLACKLIST_BLOCK_SIZE; | |
| zend_blacklist_entry *new_entries = (zend_blacklist_entry *) realloc( | |
| blacklist->entries, | |
| sizeof(zend_blacklist_entry) * blacklist->size | |
| ); | |
| if (!new_entries) { | |
| zend_accel_error_noreturn( | |
| ACCEL_LOG_FATAL, | |
| "Failed to reallocate blacklist entries\n" | |
| ); | |
| return; | |
| } | |
| blacklist->entries = new_entries; | |
| } |
🤖 Prompt for AI Agents
In opcache/zend_accelerator_blacklist.c around lines 234 to 237, the code calls
realloc without checking if it returns NULL, risking a memory leak if allocation
fails. Modify the code to store the result of realloc in a temporary pointer,
check if it is NULL, and only assign it back to blacklist->entries if
successful. If realloc returns NULL, handle the error appropriately without
losing the original pointer.
| /* strip \" */ | ||
| if (pbuf[0] == '\"' && pbuf[path_length - 1]== '\"') { | ||
| *pbuf++ = 0; | ||
| path_length -= 2; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve quote stripping validation.
The quote stripping logic assumes quotes are at both start and end but doesn't validate path_length >= 2 before accessing pbuf[path_length - 1].
/* strip \" */
- if (pbuf[0] == '\"' && pbuf[path_length - 1]== '\"') {
+ if (path_length >= 2 && pbuf[0] == '\"' && pbuf[path_length - 1] == '\"') {
*pbuf++ = 0;
path_length -= 2;
}📝 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.
| /* strip \" */ | |
| if (pbuf[0] == '\"' && pbuf[path_length - 1]== '\"') { | |
| *pbuf++ = 0; | |
| path_length -= 2; | |
| } | |
| /* strip \" */ | |
| if (path_length >= 2 && pbuf[0] == '\"' && pbuf[path_length - 1] == '\"') { | |
| *pbuf++ = 0; | |
| path_length -= 2; | |
| } |
🤖 Prompt for AI Agents
In opcache/zend_accelerator_blacklist.c around lines 278 to 282, the code strips
quotes from a string but does not check if path_length is at least 2 before
accessing pbuf[path_length - 1], which can cause out-of-bounds access. Fix this
by adding a condition to ensure path_length >= 2 before checking and stripping
the quotes at both ends.
| if (PCRE_G(jit)) { | ||
| if (0 > pcre2_jit_compile(it->re, PCRE2_JIT_COMPLETE)) { | ||
| /* Don't return here, even JIT could fail to compile, the pattern is still usable. */ | ||
| pcre2_get_error_message(errnumber, pcre_error, sizeof(pcre_error)); | ||
| zend_accel_error(ACCEL_LOG_WARNING, "Blacklist JIT compilation failed, %s\n", pcre_error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect error number usage in JIT compilation error.
The errnumber variable contains the error from pcre2_compile, not from pcre2_jit_compile. The JIT compilation error should use the return value from pcre2_jit_compile.
if (PCRE_G(jit)) {
- if (0 > pcre2_jit_compile(it->re, PCRE2_JIT_COMPLETE)) {
+ int jit_error = pcre2_jit_compile(it->re, PCRE2_JIT_COMPLETE);
+ if (jit_error < 0) {
/* Don't return here, even JIT could fail to compile, the pattern is still usable. */
- pcre2_get_error_message(errnumber, pcre_error, sizeof(pcre_error));
+ pcre2_get_error_message(jit_error, pcre_error, sizeof(pcre_error));
zend_accel_error(ACCEL_LOG_WARNING, "Blacklist JIT compilation failed, %s\n", pcre_error);
}
}📝 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 (PCRE_G(jit)) { | |
| if (0 > pcre2_jit_compile(it->re, PCRE2_JIT_COMPLETE)) { | |
| /* Don't return here, even JIT could fail to compile, the pattern is still usable. */ | |
| pcre2_get_error_message(errnumber, pcre_error, sizeof(pcre_error)); | |
| zend_accel_error(ACCEL_LOG_WARNING, "Blacklist JIT compilation failed, %s\n", pcre_error); | |
| } | |
| } | |
| if (PCRE_G(jit)) { | |
| int jit_error = pcre2_jit_compile(it->re, PCRE2_JIT_COMPLETE); | |
| if (jit_error < 0) { | |
| /* Don't return here, even JIT could fail to compile, the pattern is still usable. */ | |
| pcre2_get_error_message(jit_error, pcre_error, sizeof(pcre_error)); | |
| zend_accel_error(ACCEL_LOG_WARNING, "Blacklist JIT compilation failed, %s\n", pcre_error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In opcache/zend_accelerator_blacklist.c around lines 188 to 194, the error
number used for the JIT compilation failure message is incorrectly taken from
the pcre2_compile error instead of the pcre2_jit_compile return value. Modify
the code to capture the return value of pcre2_jit_compile into a variable and
use that variable as the error number when calling pcre2_get_error_message to
ensure the correct error details are logged.
| /* Strip ctrl-m prefix */ | ||
| pbuf = &buf[0]; | ||
| while (*pbuf == '\r') { | ||
| *pbuf++ = 0; | ||
| path_length--; | ||
| } |
There was a problem hiding this comment.
Fix ctrl-m stripping logic.
The current logic modifies pbuf but decrements path_length, which could lead to incorrect length calculations. The loop should either modify the buffer in-place or adjust the pointer correctly.
/* Strip ctrl-m prefix */
pbuf = &buf[0];
while (*pbuf == '\r') {
- *pbuf++ = 0;
+ pbuf++;
path_length--;
}📝 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.
| /* Strip ctrl-m prefix */ | |
| pbuf = &buf[0]; | |
| while (*pbuf == '\r') { | |
| *pbuf++ = 0; | |
| path_length--; | |
| } | |
| /* Strip ctrl-m prefix */ | |
| pbuf = &buf[0]; | |
| while (*pbuf == '\r') { | |
| pbuf++; | |
| path_length--; | |
| } |
🤖 Prompt for AI Agents
In opcache/zend_accelerator_blacklist.c around lines 271 to 276, the ctrl-m
stripping loop incorrectly decrements path_length while advancing pbuf, causing
potential length miscalculations. Fix this by either shifting the buffer
contents left to remove all leading '\r' characters and adjusting path_length
accordingly, or by moving the pointer without modifying the buffer and
recalculating path_length based on the new start position. Ensure path_length
accurately reflects the length of the resulting string after stripping.
Summary by CodeRabbit