php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #61537 json_encode() incorrectly truncates/discards information
Submitted: 2012-03-28 05:01 UTC Modified: 2012-05-02 07:10 UTC
Votes:9
Avg. Score:4.1 ± 1.0
Reproduced:3 of 5 (60.0%)
Same Version:1 (33.3%)
Same OS:0 (0.0%)
From: joey@php.net Assigned: aharvey
Status: Re-Opened Package: JSON related
PHP Version: 5.4.0 OS: all
Private report: No CVE-ID:
Have you experienced this issue?
Rate the importance of this bug to you:

 [2012-03-28 05:01 UTC] joey@php.net
Description:
------------
When json_encode() cannot correctly encode its argument into JSON, the expected 
behaviour is that it will return false, and people can check json_last_error() to 
see why it failed. Currently, it will replace all data it could not encode with 
the string 'null'. This can lead to what appears to be silently discarding data.

Test script:
---------------
<?php

var_dump(json_encode(chr(215)), json_last_error());


Expected result:
----------------
bool(false)
int(5)


Actual result:
--------------
string(4) 'null'
int(5)


Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-03-28 05:01 UTC] joey@php.net
Per documentation:

Return Values: Returns a JSON encoded string on success or FALSE on failure.
 [2012-03-28 05:12 UTC] aharvey@php.net
I think we've got two options here:

1. Change the documented behaviour to note that invalid UTF-8 strings are 
encoded as NULL, rather than causing json_encode() to return false, and advise 
users to check json_last_error() if they're concerned about the output.

2. Change the json_encode() behaviour to match the documentation. Quick and 
dirty POC patch which adds a switch to fall back to the current behaviour: 
https://gist.github.com/7735daabc56fb38eb511

Option 2 has a BC concern: we've never advertised json_encode() as not returning 
false in this case, but people may rely on it.

Either way, I think we should probably drop the if (PG(display_errors)) { ... } 
around the warning in json_escape_string().

Anyone else have thoughts?
 [2012-03-28 08:11 UTC] yohgaki@php.net
I prefer option 2. 
It's an error condition anyway.
 [2012-04-02 01:19 UTC] aharvey@php.net
-Status: Open +Status: Assigned -Assigned To: +Assigned To: aharvey
 [2012-04-02 01:19 UTC] aharvey@php.net
Haven't heard any screaming, so let's go with option 2.
 [2012-04-10 21:10 UTC] theanomaly dot is at gmail dot com
The patch submitted fixes the problem.

json_ecode() has been incorrectly documented all along. Instead of json_encode() 
returning false it has been gladly returning a string containing "null" to 
replace invalid data. Now, "null" is valid json, but the problem is the user 
never ends up knowing that json_encode() silently failed unless they explicitly 
check json_last_error() every single time they call json_encode(). Clearly this 
was not the documented behavior.

It should be the case that when the user tries...

<?php
if (!(json_encode() = $json)) {
    /* Then we have valid json to work with and there is no error */
} else {
    /*
        json_encode() failed and we should check json_last_error()
        in order to know why
    */
?>


I also submitted a patch to fix the problem not realizing there was another 
patch already there which fixes it too and allows it throw the error.
https://github.com/srgoogleguy/php-
src/commit/ba181fce117a3a7d1c7668cf30744b3d7cd7cdbf

However, now there is a test that's going to fail ext/json/tests/bug43941.phpt 
which is testing for undocumented behavior. It was in response to this older bug 
report:

https://bugs.php.net/bug.php?id=43941

However, that fix never really fixed the problem. It just made it worse. In my 
opinion we should remove this test. I just wanted aharvey@php.net to also look 
at in case they feel the test should remain or be modified to comply with the 
spec.

Thanks.
 [2012-04-11 00:44 UTC] aharvey@php.net
Automatic comment on behalf of adam@pwd.net.au
Revision: http://git.php.net/?p=php-src.git;a=commit;h=3f3ad30c50c32327e723046bcc6649d05dd9236a
Log: Fix bug #61537 (json_encode() incorrectly truncates/discards information) and remove a test case that's now mooted by this fix.
 [2012-04-11 00:44 UTC] aharvey@php.net
Automatic comment on behalf of adam@pwd.net.au
Revision: http://git.php.net/?p=php-src.git;a=commit;h=cb2a1c71c96d7c9b2ee03d77beae0c8e0d385f1b
Log: Fix bug #61537 (json_encode() incorrectly truncates/discards information) and remove a test case that's now mooted by this fix.
 [2012-04-11 00:44 UTC] aharvey@php.net
-Status: Assigned +Status: Closed
 [2012-04-11 00:44 UTC] aharvey@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2012-04-12 10:03 UTC] johannes@php.net
Automatic comment on behalf of adam@pwd.net.au
Revision: http://git.php.net/?p=php-src.git;a=commit;h=3f3ad30c50c32327e723046bcc6649d05dd9236a
Log: Fix bug #61537 (json_encode() incorrectly truncates/discards information) and remove a test case that's now mooted by this fix.
 [2012-05-02 07:09 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7bbd5521d28ee77c5a8df80174f52dad0112e872
Log: Revert &quot;Fix bug #61537 (json_encode() incorrectly truncates/discards information) and&quot;
 [2012-05-02 07:10 UTC] stas@php.net
-Status: Closed +Status: Re-Opened
 [2012-05-02 07:10 UTC] stas@php.net
Sorry, the fix wasn't correct, had to revert it. Please correct the wrong things 
and reapply.
 [2012-05-02 07:11 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7bbd5521d28ee77c5a8df80174f52dad0112e872
Log: Revert &quot;Fix bug #61537 (json_encode() incorrectly truncates/discards information) and&quot;
 [2012-06-20 14:29 UTC] arjen at react dot com
This fix was reverted on 5.4, but not for 5.3.14.
5.3.14 is now behaving different than 5.4.4: http://3v4l.org/eeXV2
 [2012-06-20 21:47 UTC] theanomaly dot is at gmail dot com
I had previously submitted this PR in light of this bug report, but unaware of 
aharvey at php dot net's patch I withdrew it afterwards. I'm submitting it again 
as a temporary solution since it doesn't break anything else. Should this 
functionality also provide an option for going back to the old behavior as nikic 
suggests in the PR comments? https://github.com/php/php-src/pull/111

stas at php dot net:

If so please let me know what you think...
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Fri Apr 18 03:02:48 2014 UTC