From 4afdcf5f8653847f2dbece15b820935420be61e2 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Thu, 26 Mar 2026 18:05:28 -0400 Subject: [PATCH 1/4] Fix GH-17399: iconv memory leak on bailout 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 GH-17399 --- ext/iconv/iconv.c | 210 ++++++++++++++--------- ext/iconv/tests/gh17399.phpt | 13 ++ ext/iconv/tests/gh17399_iconv.phpt | 13 ++ ext/iconv/tests/gh17399_mime_decode.phpt | 13 ++ ext/iconv/tests/gh17399_substr.phpt | 13 ++ 5 files changed, 180 insertions(+), 82 deletions(-) create mode 100644 ext/iconv/tests/gh17399.phpt create mode 100644 ext/iconv/tests/gh17399_iconv.phpt create mode 100644 ext/iconv/tests/gh17399_mime_decode.phpt create mode 100644 ext/iconv/tests/gh17399_substr.phpt diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c index ba74eddd012fb..36d081c0f2026 100644 --- a/ext/iconv/iconv.c +++ b/ext/iconv/iconv.c @@ -459,59 +459,65 @@ PHP_ICONV_API php_iconv_err_t php_iconv_string(const char *in_p, size_t in_len, out_left = in_len + 32; /* Avoid realloc() most cases */ out_size = 0; bsz = out_left; - out_buf = zend_string_alloc(bsz, 0); - out_p = ZSTR_VAL(out_buf); - - while (in_left > 0) { - result = iconv(cd, (ICONV_CONST char **) &in_p, &in_left, (char **) &out_p, &out_left); - out_size = bsz - out_left; - if (result == (size_t)(-1)) { - if (ignore_ilseq && errno == EILSEQ) { - if (in_left <= 1) { - result = 0; - } else { - errno = 0; - in_p++; - in_left--; - continue; + + zend_try { + out_buf = zend_string_alloc(bsz, 0); + out_p = ZSTR_VAL(out_buf); + + while (in_left > 0) { + result = iconv(cd, (ICONV_CONST char **) &in_p, &in_left, (char **) &out_p, &out_left); + out_size = bsz - out_left; + if (result == (size_t)(-1)) { + if (ignore_ilseq && errno == EILSEQ) { + if (in_left <= 1) { + result = 0; + } else { + errno = 0; + in_p++; + in_left--; + continue; + } } - } - if (errno == E2BIG && in_left > 0) { - /* converted string is longer than out buffer */ - bsz += in_len; + if (errno == E2BIG && in_left > 0) { + /* converted string is longer than out buffer */ + bsz += in_len; - out_buf = zend_string_extend(out_buf, bsz, 0); - out_p = ZSTR_VAL(out_buf); - out_p += out_size; - out_left = bsz - out_size; - continue; + out_buf = zend_string_extend(out_buf, bsz, 0); + out_p = ZSTR_VAL(out_buf); + out_p += out_size; + out_left = bsz - out_size; + continue; + } } + break; } - break; - } - if (result != (size_t)(-1)) { - /* flush the shift-out sequences */ - for (;;) { - result = iconv(cd, NULL, NULL, (char **) &out_p, &out_left); - out_size = bsz - out_left; + if (result != (size_t)(-1)) { + /* flush the shift-out sequences */ + for (;;) { + result = iconv(cd, NULL, NULL, (char **) &out_p, &out_left); + out_size = bsz - out_left; - if (result != (size_t)(-1)) { - break; - } + if (result != (size_t)(-1)) { + break; + } - if (errno == E2BIG) { - bsz += 16; - out_buf = zend_string_extend(out_buf, bsz, 0); - out_p = ZSTR_VAL(out_buf); - out_p += out_size; - out_left = bsz - out_size; - } else { - break; + if (errno == E2BIG) { + bsz += 16; + out_buf = zend_string_extend(out_buf, bsz, 0); + out_p = ZSTR_VAL(out_buf); + out_p += out_size; + out_left = bsz - out_size; + } else { + break; + } } } - } + } zend_catch { + iconv_close(cd); + zend_bailout(); + } zend_end_try(); iconv_close(cd); @@ -684,58 +690,68 @@ static php_iconv_err_t _php_iconv_substr(smart_str *pretval, errno = 0; more = nbytes > 0 && len > 0; - for (in_p = str, in_left = nbytes, cnt = 0; more; ++cnt) { - out_p = buf; - out_left = sizeof(buf); + zend_try { + for (in_p = str, in_left = nbytes, cnt = 0; more; ++cnt) { + out_p = buf; + out_left = sizeof(buf); - more = in_left > 0 && len > 0; + more = in_left > 0 && len > 0; - iconv(cd1, more ? (ICONV_CONST char **)&in_p : NULL, more ? &in_left : NULL, (char **) &out_p, &out_left); - if (out_left == sizeof(buf)) { - break; - } + iconv(cd1, more ? (ICONV_CONST char **)&in_p : NULL, more ? &in_left : NULL, (char **) &out_p, &out_left); + if (out_left == sizeof(buf)) { + break; + } - if ((zend_long)cnt >= offset) { - if (cd2 == (iconv_t)NULL) { - cd2 = iconv_open(enc, GENERIC_SUPERSET_NAME); + if ((zend_long)cnt >= offset) { + if (cd2 == (iconv_t)NULL) { + cd2 = iconv_open(enc, GENERIC_SUPERSET_NAME); - if (cd2 == (iconv_t)(-1)) { - cd2 = (iconv_t)NULL; - if (errno == EINVAL) { - err = PHP_ICONV_ERR_WRONG_CHARSET; - } else { - err = PHP_ICONV_ERR_CONVERTER; + if (cd2 == (iconv_t)(-1)) { + cd2 = (iconv_t)NULL; + if (errno == EINVAL) { + err = PHP_ICONV_ERR_WRONG_CHARSET; + } else { + err = PHP_ICONV_ERR_CONVERTER; + } + break; } + } + + if (_php_iconv_appendl(pretval, buf, sizeof(buf), cd2) != PHP_ICONV_ERR_SUCCESS) { break; } + --len; } - if (_php_iconv_appendl(pretval, buf, sizeof(buf), cd2) != PHP_ICONV_ERR_SUCCESS) { - break; - } - --len; } - } - - switch (errno) { - case EINVAL: - err = PHP_ICONV_ERR_ILLEGAL_CHAR; - break; + switch (errno) { + case EINVAL: + err = PHP_ICONV_ERR_ILLEGAL_CHAR; + break; - case EILSEQ: - err = PHP_ICONV_ERR_ILLEGAL_SEQ; - break; + case EILSEQ: + err = PHP_ICONV_ERR_ILLEGAL_SEQ; + break; - case E2BIG: - break; - } - if (err == PHP_ICONV_ERR_SUCCESS) { + case E2BIG: + break; + } + if (err == PHP_ICONV_ERR_SUCCESS) { + if (cd2 != (iconv_t)NULL) { + _php_iconv_appendl(pretval, NULL, 0, cd2); + } + smart_str_0(pretval); + } + } zend_catch { + if (cd1 != (iconv_t)NULL) { + iconv_close(cd1); + } if (cd2 != (iconv_t)NULL) { - _php_iconv_appendl(pretval, NULL, 0, cd2); + iconv_close(cd2); } - smart_str_0(pretval); - } + zend_bailout(); + } zend_end_try(); if (cd1 != (iconv_t)NULL) { iconv_close(cd1); @@ -942,6 +958,8 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn goto out; } + buf = safe_emalloc(1, max_line_len, 5); + cd_pl = iconv_open(ICONV_ASCII_ENCODING, enc); if (cd_pl == (iconv_t)(-1)) { if (errno == EINVAL) { @@ -962,10 +980,10 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn goto out; } - buf = safe_emalloc(1, max_line_len, 5); - char_cnt = max_line_len; + zend_try { + _php_iconv_appendl(pretval, fname, fname_nbytes, cd_pl); char_cnt -= fname_nbytes; smart_str_appendl(pretval, ": ", sizeof(": ") - 1); @@ -1172,6 +1190,22 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn smart_str_0(pretval); + } zend_catch { + if (cd != (iconv_t)(-1)) { + iconv_close(cd); + } + if (cd_pl != (iconv_t)(-1)) { + iconv_close(cd_pl); + } + if (encoded != NULL) { + zend_string_release_ex(encoded, 0); + } + if (buf != NULL) { + efree(buf); + } + zend_bailout(); + } zend_end_try(); + out: if (cd != (iconv_t)(-1)) { iconv_close(cd); @@ -1224,6 +1258,7 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st } p1 = str; + zend_try { for (str_left = str_nbytes; str_left > 0; str_left--, p1++) { int eos = 0; @@ -1729,6 +1764,17 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st } smart_str_0(pretval); + + } zend_catch { + if (cd != (iconv_t)(-1)) { + iconv_close(cd); + } + if (cd_pl != (iconv_t)(-1)) { + iconv_close(cd_pl); + } + zend_bailout(); + } zend_end_try(); + out: if (cd != (iconv_t)(-1)) { iconv_close(cd); diff --git a/ext/iconv/tests/gh17399.phpt b/ext/iconv/tests/gh17399.phpt new file mode 100644 index 0000000000000..078bfe2c39ce7 --- /dev/null +++ b/ext/iconv/tests/gh17399.phpt @@ -0,0 +1,13 @@ +--TEST-- +GH-17399 (iconv memory leak with large line-length in iconv_mime_encode) +--EXTENSIONS-- +iconv +--FILE-- + PHP_INT_MAX, +); +iconv_mime_encode('Subject', 'test', $options); +?> +--EXPECTF-- +Fatal error: Allowed memory size of %d bytes exhausted %s in %s on line %d diff --git a/ext/iconv/tests/gh17399_iconv.phpt b/ext/iconv/tests/gh17399_iconv.phpt new file mode 100644 index 0000000000000..2cdd0e176b20b --- /dev/null +++ b/ext/iconv/tests/gh17399_iconv.phpt @@ -0,0 +1,13 @@ +--TEST-- +GH-17399 (iconv() leak on bailout) +--EXTENSIONS-- +iconv +--INI-- +memory_limit=2M +--FILE-- + +--EXPECTF-- +Fatal error: Allowed memory size of %d bytes exhausted %s in %s on line %d diff --git a/ext/iconv/tests/gh17399_mime_decode.phpt b/ext/iconv/tests/gh17399_mime_decode.phpt new file mode 100644 index 0000000000000..95a37d364f331 --- /dev/null +++ b/ext/iconv/tests/gh17399_mime_decode.phpt @@ -0,0 +1,13 @@ +--TEST-- +GH-17399 (iconv_mime_decode() leak on bailout) +--EXTENSIONS-- +iconv +--INI-- +memory_limit=2M +--FILE-- + +--EXPECTF-- +Fatal error: Allowed memory size of %d bytes exhausted %s in %s on line %d diff --git a/ext/iconv/tests/gh17399_substr.phpt b/ext/iconv/tests/gh17399_substr.phpt new file mode 100644 index 0000000000000..35d791db59f06 --- /dev/null +++ b/ext/iconv/tests/gh17399_substr.phpt @@ -0,0 +1,13 @@ +--TEST-- +GH-17399 (iconv_substr() leak on bailout) +--EXTENSIONS-- +iconv +--INI-- +memory_limit=2M +--FILE-- + +--EXPECTF-- +Fatal error: Allowed memory size of %d bytes exhausted %s in %s on line %d From 12b48b93676cebee1abede02734061de21dee2c0 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Fri, 27 Mar 2026 11:19:24 -0400 Subject: [PATCH 2/4] Address review feedback: use bailout flag instead of duplicating cleanup 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. --- ext/iconv/iconv.c | 31 ++++++++++--------------------- ext/iconv/tests/gh17399.phpt | 4 ++-- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c index 36d081c0f2026..9c768e3883be6 100644 --- a/ext/iconv/iconv.c +++ b/ext/iconv/iconv.c @@ -982,8 +982,8 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn char_cnt = max_line_len; + bool bailout = false; zend_try { - _php_iconv_appendl(pretval, fname, fname_nbytes, cd_pl); char_cnt -= fname_nbytes; smart_str_appendl(pretval, ": ", sizeof(": ") - 1); @@ -1191,19 +1191,7 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn smart_str_0(pretval); } zend_catch { - if (cd != (iconv_t)(-1)) { - iconv_close(cd); - } - if (cd_pl != (iconv_t)(-1)) { - iconv_close(cd_pl); - } - if (encoded != NULL) { - zend_string_release_ex(encoded, 0); - } - if (buf != NULL) { - efree(buf); - } - zend_bailout(); + bailout = true; } zend_end_try(); out: @@ -1219,6 +1207,9 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn if (buf != NULL) { efree(buf); } + if (bailout) { + zend_bailout(); + } return err; } /* }}} */ @@ -1258,6 +1249,7 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st } p1 = str; + bool bailout = false; zend_try { for (str_left = str_nbytes; str_left > 0; str_left--, p1++) { int eos = 0; @@ -1766,13 +1758,7 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st smart_str_0(pretval); } zend_catch { - if (cd != (iconv_t)(-1)) { - iconv_close(cd); - } - if (cd_pl != (iconv_t)(-1)) { - iconv_close(cd_pl); - } - zend_bailout(); + bailout = true; } zend_end_try(); out: @@ -1782,6 +1768,9 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st if (cd_pl != (iconv_t)(-1)) { iconv_close(cd_pl); } + if (bailout) { + zend_bailout(); + } return err; } /* }}} */ diff --git a/ext/iconv/tests/gh17399.phpt b/ext/iconv/tests/gh17399.phpt index 078bfe2c39ce7..b93fc638e84cd 100644 --- a/ext/iconv/tests/gh17399.phpt +++ b/ext/iconv/tests/gh17399.phpt @@ -4,9 +4,9 @@ GH-17399 (iconv memory leak with large line-length in iconv_mime_encode) iconv --FILE-- PHP_INT_MAX, -); +]; iconv_mime_encode('Subject', 'test', $options); ?> --EXPECTF-- From 5dd981555285706d2a9e8a298f323cbb2e7ae78e Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Fri, 27 Mar 2026 11:28:22 -0400 Subject: [PATCH 3/4] Move bailout declarations to function scope to fix -Wsometimes-uninitialized 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. --- ext/iconv/iconv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c index 9c768e3883be6..bafa74e2d6729 100644 --- a/ext/iconv/iconv.c +++ b/ext/iconv/iconv.c @@ -920,6 +920,7 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn { php_iconv_err_t err = PHP_ICONV_ERR_SUCCESS; iconv_t cd = (iconv_t)(-1), cd_pl = (iconv_t)(-1); + bool bailout = false; size_t char_cnt = 0; size_t out_charset_len; size_t lfchars_len; @@ -982,7 +983,6 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn char_cnt = max_line_len; - bool bailout = false; zend_try { _php_iconv_appendl(pretval, fname, fname_nbytes, cd_pl); char_cnt -= fname_nbytes; @@ -1218,6 +1218,7 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *str, size_t str_nbytes, const char *enc, const char **next_pos, int mode) { php_iconv_err_t err = PHP_ICONV_ERR_SUCCESS; + bool bailout = false; iconv_t cd = (iconv_t)(-1), cd_pl = (iconv_t)(-1); @@ -1249,7 +1250,6 @@ static php_iconv_err_t _php_iconv_mime_decode(smart_str *pretval, const char *st } p1 = str; - bool bailout = false; zend_try { for (str_left = str_nbytes; str_left > 0; str_left--, p1++) { int eos = 0; From 21c3a244755c8599ba533017f9758970bcfcfd72 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Fri, 27 Mar 2026 14:49:48 -0400 Subject: [PATCH 4/4] Move safe_emalloc back into zend_try, remove stray blank line --- ext/iconv/iconv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c index bafa74e2d6729..dd6003b1c7559 100644 --- a/ext/iconv/iconv.c +++ b/ext/iconv/iconv.c @@ -959,8 +959,6 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn goto out; } - buf = safe_emalloc(1, max_line_len, 5); - cd_pl = iconv_open(ICONV_ASCII_ENCODING, enc); if (cd_pl == (iconv_t)(-1)) { if (errno == EINVAL) { @@ -984,6 +982,7 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn char_cnt = max_line_len; zend_try { + buf = safe_emalloc(1, max_line_len, 5); _php_iconv_appendl(pretval, fname, fname_nbytes, cd_pl); char_cnt -= fname_nbytes; smart_str_appendl(pretval, ": ", sizeof(": ") - 1); @@ -1189,7 +1188,6 @@ static php_iconv_err_t _php_iconv_mime_encode(smart_str *pretval, const char *fn } while (in_left > 0); smart_str_0(pretval); - } zend_catch { bailout = true; } zend_end_try();