Repository navigation
iconv memory leak #17399
Description
Description
The following code:
<?php
$options = array(
'line-length' => PHP_INT_MAX,
);
echo iconv_mime_encode('Subject', $text, $options);Resulted in this output:
==965243==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 112 byte(s) in 1 object(s) allocated from:
#0 0x6807ed in malloc (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x6807ed)
#1 0x7f9ba81b276c in __gconv_open iconv/./iconv/gconv_open.c:77:28
#2 0x7f9ba81b22b7 in iconv_open iconv/./iconv/iconv_open.c:39:13
#3 0x60200001994f (<unknown module>)
Direct leak of 112 byte(s) in 1 object(s) allocated from:
#0 0x6807ed in malloc (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x6807ed)
#1 0x7f9ba81b276c in __gconv_open iconv/./iconv/gconv_open.c:77:28
#2 0x7f9ba81b22b7 in iconv_open iconv/./iconv/iconv_open.c:39:13
#3 0x6020000198cf (<unknown module>)
Indirect leak of 32640 byte(s) in 1 object(s) allocated from:
#0 0x6807ed in malloc (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x6807ed)
#1 0x7f9ba81b27e6 in __gconv_open iconv/./iconv/gconv_open.c:127:36
#2 0x7f9ba81b22b7 in iconv_open iconv/./iconv/iconv_open.c:39:13
#3 0x6020000198cf (<unknown module>)
Indirect leak of 32640 byte(s) in 1 object(s) allocated from:
#0 0x6807ed in malloc (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x6807ed)
#1 0x7f9ba81b27e6 in __gconv_open iconv/./iconv/gconv_open.c:127:36
#2 0x7f9ba81b22b7 in iconv_open iconv/./iconv/iconv_open.c:39:13
#3 0x60200001994f (<unknown module>)
Indirect leak of 416 byte(s) in 2 object(s) allocated from:
#0 0x6807ed in malloc (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x6807ed)
#1 0x7f9ba81bd9d6 in __gconv_lookup_cache iconv/./iconv/gconv_cache.c:365:36
SUMMARY: AddressSanitizer: 65920 byte(s) leaked in 6 allocation(s).
PHP Version
nightly
Operating System
No response
Activity
devnexen commentedon Jan 8, 2025
well there is an ZendMM out of memory triggered with the line length, so the iconv flows do not get freed, that is expected. Either we put len checks but then it defeats the usefulness of safe emalloc here or leave it as is. cc @nielsdos thoughts ?
cmb69 commentedon Jan 8, 2025
php-src/ext/iconv/iconv.c
Line 965 in cee64ed
We could move up that allocation prior to the first
iconv_open()call.devnexen commentedon Jan 8, 2025
Yes I was starting to think it too but you did it way faster than me, feel free to open a PR :)
ndossche commentedon Jan 8, 2025
Fine by me.
That would "fix" this issue, but would not fix the general problem and I'm sure you could still trigger the same issue via other means. For example in this code, if one of the smart_str operations causes a bailout you have the exact same problem.
The core problem is that iconv uses the system allocator, so on bailout we don't free the data. This is a general problem in many extensions.
cmb69 commentedon Jan 23, 2025
Hmm, given that this has been found by fuzzing, maybe we should add
iconv_open(and maybe some other functions) to lsan-suppressions.txt?ndossche commentedon Jan 23, 2025
That might hide real leaks too. I think it's more important to keep reporting such leaks than risking false negatives.
Fix phpGH-17399: iconv memory leak with large line-length
Fix phpGH-17399: iconv memory leak on bailout
Merge branch 'PHP-8.4' into PHP-8.5
Merge branch 'PHP-8.5'
Fix GH-17399: iconv memory leak on bailout