iconv memory leak

[YuanchengJiang]

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

[devnexen]

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]

php-src/ext/iconv/iconv.c

Line 965 in cee64ed

buf = safe_emalloc(1, max_line_len, 5);

We could move up that allocation prior to the first iconv_open() call.

[devnexen]

Yes I was starting to think it too but you did it way faster than me, feel free to open a PR :)

[nielsdos]

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]

Hmm, given that this has been found by fuzzing, maybe we should add iconv_open (and maybe some other functions) to lsan-suppressions.txt?

[nielsdos]

That might hide real leaks too. I think it's more important to keep reporting such leaks than risking false negatives.