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 4 commits intophp:PHP-8.4from
Conversation
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
As this is a bugfix this wouldn't require RM approval. :)
iliaal
force-pushed
the
fix/gh-17399-iconv-leak
branch
from
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. |
iliaal
force-pushed
the
fix/gh-17399-iconv-leak
branch
from
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
iliaal
force-pushed
the
fix/gh-17399-iconv-leak
branch
from
8f71d79 to
4afdcf5
Compare
|
@ndossche I updated the PR using |
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Luckily, the indentation applies cleanly to higher branches. LGTM otherwise.
ext/iconv/iconv.c
Outdated
| char_cnt = max_line_len; | ||
|
|
||
| zend_try { | ||
|
|
| } | ||
|
|
||
| smart_str_0(pretval); | ||
|
|
There was a problem hiding this comment.
Same here. That's what the majority of the uses of zend_try/catch do.
Use a bool flag in zend_catch and let the existing out: cleanup path handle resource freeing, then conditionally call zend_bailout() after. Also fix test to use short array syntax and remove stray blank line.
…ialized The bailout variable was declared after goto targets, so paths jumping to out: via goto could reach the if (bailout) check without the variable being initialized.
|
@iluuu1994 I think I got all the CS recommendation covered 🤞 |
ext/iconv/iconv.c
Outdated
| goto out; | ||
| } | ||
|
|
||
| buf = safe_emalloc(1, max_line_len, 5); |
There was a problem hiding this comment.
This should be moved back (and into the zend_try).
|
|
||
| smart_str_0(pretval); | ||
|
|
||
| } zend_catch { |
There was a problem hiding this comment.
The newline above should be removed.
| { | ||
| php_iconv_err_t err = PHP_ICONV_ERR_SUCCESS; | ||
| iconv_t cd = (iconv_t)(-1), cd_pl = (iconv_t)(-1); | ||
| bool bailout = false; |
There was a problem hiding this comment.
I'd prefer to place this directly above zend_try.
There was a problem hiding this comment.
I left that one out, it triggered -Wsometimes-uninitialized on clang before, so while it could be moved a bit lower, this seem ok to me.
4 participants
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