php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #72320 iconv_substr returns false for empty strings
Submitted: 2016-06-02 21:00 UTC Modified: 2016-06-03 12:34 UTC
From: chris at ocproducts dot com Assigned:
Status: Closed Package: ICONV related
PHP Version: 7.0.7 OS:
Private report: No CVE-ID:
 [2016-06-02 21:00 UTC] chris at ocproducts dot com
Description:
------------
From iconv.c...

_php_iconv_substr returns success if the offset is more than or equal to the character length of the input string...

	if ((size_t)offset >= total_len) {
		return PHP_ICONV_ERR_SUCCESS;
	}

...

Then in the calling function (iconv_substr) we check the result...

	if (err == PHP_ICONV_ERR_SUCCESS && ZSTR_LEN(str) > 0 && retval.s != NULL) {
		RETURN_NEW_STR(retval.s);
	}
	smart_str_free(&retval);
	RETURN_FALSE;

Note how this will always return false for an empty input string. Additionally it will also return false if offset==total_len, because retval.s will not have been set at the point of _php_iconv_substr returning (as indicated).

The documentation says:

"If str is shorter than offset characters long, FALSE will be returned."

I believe the documentation describes the correct behaviour. 0 characters is not shorter than a 0 offset.

mb_substr and substr are confirmed to work correctly.

Likely nobody has noticed this bug until now because false and '' are similar, but it matters for us strict type users, and when interacting with persistency layers.

Test script:
---------------
var_dump(iconv_substr('',0,10,'utf-8'));

Expected result:
----------------
string(0) ""

Actual result:
--------------
bool(false)

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-06-03 12:34 UTC] cmb@php.net
-Status: Open +Status: Verified
 [2016-06-03 12:34 UTC] cmb@php.net
The behavior of substr() has been deliberately changed as of PHP
7.0.0 (see <https://3v4l.org/Q52RB> and the PHP manual[1]). Hence,
the behavior of iconv_substr() should also have been changed, and
not having already done so is an oversight, in my opinion.
(mb_substr() already worked this way.)

[1] <http://php.net/manual/en/migration70.changed-functions.php#migration70.changed-functions.core>
 [2016-08-29 23:25 UTC] cmb@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=php-src.git;a=commit;h=a837b2f6a35ab7cdfb49b9407ac0401caa672b23
Log: Fix #72320: iconv_substr returns false for empty strings
 [2016-08-29 23:25 UTC] cmb@php.net
-Status: Verified +Status: Closed
 [2016-10-17 10:10 UTC] bwoebi@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=php-src.git;a=commit;h=a837b2f6a35ab7cdfb49b9407ac0401caa672b23
Log: Fix #72320: iconv_substr returns false for empty strings
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC