-
Notifications
You must be signed in to change notification settings - Fork 20
fix: prevent cURL resource leak in Adapter::call() #59
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
Conversation
Added try-finally block to ensure curl_close() is always called, even when exceptions are thrown. Previously, cURL handles would leak when curl_errno() or HTTP error status checks threw exceptions before curl_close() was reached.
WalkthroughThe changes refactor curl resource handling in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Analytics/Adapter.php (1)
167-171: Edge case: Content-Type without semicolon won't be parsed correctly.If
$responseTypeis"application/json"(no charset suffix),strpos($responseType, ';')returnsfalse, causingsubstr()to return an empty string. This would skip JSON decoding.Consider using
explodeorstr_containsfor more robust handling:- switch (substr($responseType, 0, strpos($responseType, ';'))) { - case 'application/json': - $responseBody = json_decode($responseBody, true); - break; - } + $mimeType = strstr($responseType, ';', true) ?: $responseType; + if ($mimeType === 'application/json') { + $responseBody = json_decode($responseBody, true); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Analytics/Adapter.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/Analytics/Adapter.php
[error] 181-181: HTTP 401 Unauthorized from Plausible API: Invalid API key.
🪛 PHPMD (2.15.0)
src/Analytics/Adapter.php
145-145: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (1)
src/Analytics/Adapter.php (1)
139-188: LGTM! The try-finally pattern correctly ensures cURL resource cleanup.The fix properly addresses the resource leak by guaranteeing
curl_close($ch)executes regardless of whether exceptions are thrown on error paths (curl_errno or HTTP status >= 400).
| $responseBody = json_decode($responseBody, true); | ||
| break; | ||
| } | ||
| $responseType = $responseHeaders['Content-Type'] ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case mismatch: header key stored lowercase but accessed with mixed case.
Line 153 stores headers with strtolower(), but this line accesses with 'Content-Type'. PHP arrays are case-sensitive, so this will always return the default empty string, preventing JSON response parsing.
- $responseType = $responseHeaders['Content-Type'] ?? '';
+ $responseType = $responseHeaders['content-type'] ?? '';📝 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.
| $responseType = $responseHeaders['Content-Type'] ?? ''; | |
| $responseType = $responseHeaders['content-type'] ?? ''; |
🤖 Prompt for AI Agents
In src/Analytics/Adapter.php around line 164, the code reads
$responseHeaders['Content-Type'] but headers were normalized to lowercase
earlier, so that key will never match; update the access to use the lowercase
key (e.g. 'content-type') or call strtolower on the looked-up key, so the
Content-Type value is retrieved correctly and JSON responses can be parsed.
Summary
Adapter::call()method where cURL handles were not being closed when exceptions were throwncurl_close()is always calledProblem
The
call()method insrc/Analytics/Adapter.phpcreates a cURL handle but throws exceptions before reachingcurl_close()in two scenarios:curl_errno($ch)detects an error (line 173)This causes cURL resources to leak over time, leading to memory issues in long-running applications.
Solution
Added a try-finally block around the cURL operations to guarantee that
curl_close($ch)is always executed, even when exceptions are thrown.Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.