Fix GH-17399: iconv memory leak with large line-length#21541
Fix GH-17399: iconv memory leak with large line-length#21541iliaal wants to merge 1 commit intophp:PHP-8.4from
Conversation
Girgias
left a comment
There was a problem hiding this comment.
As this is a bugfix this wouldn't require RM approval. :)
a9eeac2 to
9604e4d
Compare
@Girgias Updated to be against 8.4, once tests pass, could you please merge, I still don't have access to merge myself 😢 |
|
This doesn't fix the real issue as I noted in the issue report. This just fixes one particular case of it. |
One issue at a time, there is no universal fix to allocations on interruption, best is to identify the underlying issues and resolve them one by one. Not sure holding a fix to a specific issue because not all issues can be fixed, makes sense. But that's just my opinion... I'll see if I can do handler for other cases @ least inside iconv() though. |
9604e4d to
8f71d79
Compare
Move the buf allocation in _php_iconv_mime_encode() before the iconv_open() calls so an OOM bailout from safe_emalloc does not leak iconv handles. Additionally, wrap bailable sections in php_iconv_string(), _php_iconv_substr(), _php_iconv_mime_encode(), and _php_iconv_mime_decode() with zend_try/zend_catch to ensure iconv handles allocated via system malloc are closed if a Zend OOM bailout fires during smart_str or zend_string operations. Closes phpGH-17399
8f71d79 to
4afdcf5
Compare
|
@ndossche I updated the PR using |
Summary
_php_iconv_mime_encode()opens twoiconv_thandles via systemmalloc, then callssafe_emalloc(1, max_line_len, 5). Whenmax_line_lenisPHP_INT_MAX, the allocation triggers an OOM bailout that skipsiconv_close()cleanup, leaking both handles.Fix: move
safe_emallocbefore bothiconv_open()calls so a bailout happens before any handles exist.Additionally, four iconv functions (
php_iconv_string,_php_iconv_substr,_php_iconv_mime_encode,_php_iconv_mime_decode) have the same class of leak:smart_strorzend_stringoperations betweeniconv_open()andiconv_close()can trigger OOM bailouts that skip cleanup. These are now wrapped withzend_try/zend_catchto close iconv handles before re-throwing.@cmb69 suggested the reordering approach in the issue thread.
Fixes #17399