# json_encode() behavior for incorrectly encoded strings - Thread: https://externals.io/message/60908 - Original archive: https://news-web.php.net/php.internals/60908 - Started by Nikita Popov on 2012-06-20T22:21:50+00:00 - Messages: 29 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/60908.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #60908 — Nikita Popov — 2012-06-20T22:21:50+00:00 URL: https://externals.io/message/60908#60908 Hi internals! We currently have a big mess concerning the behavior of `json_encode()` with incorrectly encoded UTF-8 strings. To summarize the situation: PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8 string is encountered: * The invalid string is replaced with "null", thus creating a partial JSON serialization * `json_encode()` returns the partial serialization, *not* false as it should * A warning is thrown, but only if display_errors=off * `json_last_error()` returns JSON_ERROR_UTF8 PHP 5.3.14 behaves as follows: * `json_encode()` returns false, as it should * A warning is thrown always, even with display_errors=on * `json_last_error()` returns JSON_ERROR_UTF8 * If the PHP_JSON_PARTIAL_OUTPUT_ON_ERROR option is specified, the old behavior is restored The reason for this situation is that a patch was applied for all branches, and then reverted, because Stas didn't consider the change towards always throwing a warning (even with display_errors=on) appropriate without further discussion. The backout though was done only on PHP 5.4 and master, but *not* on the PHP 5.3 branch. Thus we now have differing behavior between 5.3 and 5.4 (and PHP <= 5.3.13). So, I'd like to ask whether the patch can be reapplied to 5.4 and master or whether we should do something else to solve this problem. Nikita --- ## #60913 — Pierre Joye — 2012-06-21T06:47:31+00:00 URL: https://externals.io/message/60908#60913 · In reply to: #60908 hi, On Thu, Jun 21, 2012 at 12:21 AM, Nikita Popov wrote: > We currently have a big mess concerning the behavior of `json_encode()` > with incorrectly encoded UTF-8 strings. > > To summarize the situation: > > PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8 > string is encountered: >  * The invalid string is replaced with "null", thus creating a partial > JSON serialization >  * `json_encode()` returns the partial serialization, *not* false as it should >  * A warning is thrown, but only if display_errors=off >  * `json_last_error()` returns JSON_ERROR_UTF8 > > PHP 5.3.14 behaves as follows: >  * `json_encode()` returns false, as it should >  * A warning is thrown always, even with display_errors=on >  * `json_last_error()` returns JSON_ERROR_UTF8 >  * If the PHP_JSON_PARTIAL_OUTPUT_ON_ERROR option is specified, the > old behavior is restored > > The reason for this situation is that a patch was applied for all > branches, and then reverted, because Stas didn't consider the change > towards always throwing a warning (even with display_errors=on) > appropriate without further discussion. Well, not sure we have to discuss why such thing is bad. display_errors must be respected. Also I suspect that this change should not even be merged in 5.4 nor 5.3. At least not without real testing. > The backout though was done only on PHP 5.4 and master, but *not* on > the PHP 5.3 branch. > > Thus we now have differing behavior between 5.3 and 5.4 (and PHP <= 5.3.13). > > So, I'd like to ask whether the patch can be reapplied to 5.4 and > master or whether we should do something else to solve this problem. 5.3 should be brought in line with 5.4. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #60914 — Laruence — 2012-06-21T06:51:15+00:00 URL: https://externals.io/message/60908#60914 · In reply to: #60913 > hi, > > On Thu, Jun 21, 2012 at 12:21 AM, Nikita Popov > wrote: > > > […] > > Well, not sure we have to discuss why such thing is bad. > display_errors must be respected. Agree, and it shall be, no any exception. thanks > > Also I suspect that this change should not even be merged in 5.4 nor > 5.3. At least not without real testing. > > > […] > > 5.3 should be brought in line with 5.4. > > Cheers, > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #60917 — Nikita Popov — 2012-06-21T09:21:07+00:00 URL: https://externals.io/message/60908#60917 · In reply to: #60913 > > […] > > Well, not sure we have to discuss why such thing is bad. > display_errors must be respected. This kind of very ugly error handling is currently used in several places. It is meant to protect server administrators that are running production servers with display_errors=On from leaking information about encoding issues (which could be valuable to attackers). So the error is only thrown if it isn't shown to the client but logged instead. Personally I really don't like this, but I agree with Stas that this is a security relevant issue and shouldn't be simply changed without further discussion. > 5.3 should be brought in line with 5.4. So you think that the corrected code should be backed out from PHP 5.3 and the incorrect 5.4/master behavior be restored? Thanks, Nikita --- ## #60922 — Pierre Joye — 2012-06-21T12:37:06+00:00 URL: https://externals.io/message/60908#60922 · In reply to: #60917 hi, On Thu, Jun 21, 2012 at 11:21 AM, Nikita Popov wrote: > > […] > So you think that the corrected code should be backed out from PHP 5.3 > and the incorrect 5.4/master behavior be restored? For the warning yes. For the rest, it should imo return false as well, partial encoding or magic replacements make no sense here. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #60923 — Daniel Convissor — 2012-06-21T14:12:41+00:00 URL: https://externals.io/message/60908#60923 · In reply to: #60908 Hi Nikita: > PHP <= 5.3.13, PHP 5.4, master behave as follows when an invalid UTF-8 > string is encountered: > * A warning is thrown, but only if display_errors=off So the warning is put into the error log? But only if display_errors is off? And if display_errors is on, no warning is produced? That's very counter-intuitive and un-PHP-like. Someone mentioned it's for security reasons. By that logic, we should get rid of all error messages because they all reveal some information that can be used by attackers. Thanks, --Dan -- T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y data intensive web and database programming http://www.AnalysisAndSolutions.com/ 4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 --- ## #60924 — Nikita Popov — 2012-06-21T14:25:49+00:00 URL: https://externals.io/message/60908#60924 · In reply to: #60923 On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor wrote: > Hi Nikita: > > > […] > > So the warning is put into the error log?  But only if display_errors is > off?  And if display_errors is on, no warning is produced? > > That's very counter-intuitive and un-PHP-like.  Someone mentioned it's for > security reasons.  By that logic, we should get rid of all error > messages because they all reveal some information that can be used by > attackers. Yes, I agree with this, but other people thing differently about it ;) Personally I'd actually remove the warning completely with the return false behavior. The return value makes clear that there was an error and `json_last_error()` can be used to determine what exactly went wrong. The warning seems redundant in this case and could actually make the use more complicated. Nikita --- ## #60925 — Laruence — 2012-06-21T14:58:56+00:00 URL: https://externals.io/message/60908#60925 · In reply to: #60924 On Thu, Jun 21, 2012 at 10:25 PM, Nikita Popov wrote: > On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor > wrote: > > […] > > Yes, I agree with this, but other people thing differently about it ;) I think I must missed some infos, I though you was talking that you want to generate wanrings without regard of EG(display_errors). thanks > > Personally I'd actually remove the warning completely with the return > false behavior. The return value makes clear that there was an error > and `json_last_error()` can be used to determine what exactly went > wrong. The warning seems redundant in this case and could actually > make the use more complicated. > > Nikita > > -- > > > -- Laruence  Xinchen Hui http://www.laruence.com/ --- ## #60926 — Rasmus Lerdorf — 2012-06-21T15:33:51+00:00 URL: https://externals.io/message/60908#60926 · In reply to: #60924 > On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor > wrote: > > […] > > Yes, I agree with this, but other people thing differently about it ;) > > Personally I'd actually remove the warning completely with the return > false behavior. The return value makes clear that there was an error > and `json_last_error()` can be used to determine what exactly went > wrong. The warning seems redundant in this case and could actually > make the use more complicated. The problem with a warning here is that there is usually no way to prevent it short of using @ or preceding all calls to `htmlspecialchars()` with an `iconv()` call. A bad guy can simply send invalid UTF-8 bytes to a web app and look for that warning to get a really good idea about the server software being used. And yes, I know people should have display_errors off in production, but this case is slightly different because it is so universal. Other user-triggerable warnings are very code-dependent and there is no universal trigger string you can send to all PHP apps. Almost all PHP apps call `htmlspecialchars()` on user input at some point. The call here was whether to throw a warning at all and the idea was that it was ok to do so as long as it didn't leak to the world for improperly configured servers that had display_errors on. I would be fine with removing the warning altogether since sanitizing the string is an inherent characteristic of the function now and throwing warnings about the normal operation of a function doesn't make much sense. -Rasmus --- ## #60927 — Pierre Joye — 2012-06-21T15:40:08+00:00 URL: https://externals.io/message/60908#60927 · In reply to: #60923 On Thu, Jun 21, 2012 at 4:12 PM, Daniel Convissor wrote: > Hi Nikita: > > > […] > > So the warning is put into the error log?  But only if display_errors is > off?  And if display_errors is on, no warning is produced? damned :) I mean only display_errors is ON. obviously :) -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #60929 — Pierre Joye — 2012-06-21T18:39:04+00:00 URL: https://externals.io/message/60908#60929 · In reply to: #60926 hi Rasmus, > The problem with a warning here is that there is usually no way to > prevent it short of using @ or preceding all calls to `htmlspecialchars()` > with an `iconv()` call. A bad guy can simply send invalid UTF-8 bytes to a > web app and look for that warning to get a really good idea about the > server software being used. And yes, I know people should have > display_errors off in production, but this case is slightly different > because it is so universal. Other user-triggerable warnings are very > code-dependent and there is no universal trigger string you can send to > all PHP apps. Almost all PHP apps call `htmlspecialchars()` on user input > at some point. I have no problem to raise a warning here, but it must respect display_error. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #60945 — Sherif Ramadan — 2012-06-23T10:50:01+00:00 URL: https://externals.io/message/60908#60945 · In reply to: #60926 > > The problem with a warning here is that there is usually no way to > prevent it short of using @ or preceding all calls to `htmlspecialchars()` > with an `iconv()` call. A bad guy can simply send invalid UTF-8 bytes to a > web app and look for that warning to get a really good idea about the > server software being used. And yes, I know people should have > display_errors off in production, but this case is slightly different > because it is so universal. Other user-triggerable warnings are very > code-dependent and there is no universal trigger string you can send to > all PHP apps. Almost all PHP apps call `htmlspecialchars()` on user input > at some point. > > The call here was whether to throw a warning at all and the idea was > that it was ok to do so as long as it didn't leak to the world for > improperly configured servers that had display_errors on. I would be > fine with removing the warning altogether since sanitizing the string is > an inherent characteristic of the function now and throwing warnings > about the normal operation of a function doesn't make much sense. > > -Rasmus So in other words this patch I submitted should be fine? https://github.com/php/php-src/pull/111 This particular patch doesn't raise any controversy since it doesn't touch any of that other stuff and simply puts the defined behavior back in place as it should have been. --- ## #60946 — Nikita Popov — 2012-06-23T19:24:43+00:00 URL: https://externals.io/message/60908#60946 · In reply to: #60945 I applied a few changes in https://github.com/php/php-src/commit/84fe2cc890e49f40bac7c3ba74b3cfc6dc4cef2f and https://github.com/php/php-src/commit/36fa17a5fae84ab332366a202f0a709279a2466a. From the commit message: <<<<< `json_encode()` now returns bool(false) for all possible errors, throws the respective warning and also sets the respective `json_last_error()` error code. Three new error codes have been added: * `JSON_ERROR_RECURSION` * `JSON_ERROR_INF_OR_NAN` * `JSON_ERROR_UNSUPPORTED_TYPE` To get a partial JSON output instead of bool(false) the option `JSON_PARTIAL_OUTPUT_ON_ERROR` can be specified. In this case the invalid segments will be replaced either by null (for recursion, unsupported type and invalid JSON) or 0 (for Inf and NaN). The warning for invalid UTF-8 stays intact and is thrown also with display_errors = On. If this behavior is undesired this can be remedied later. > > […] So all branches now behave the same: bool(false) is returned, a warning is thrown and an error code is set. For now I chose to always throw the warning, but if something else comes out in conclusion to this discussion I'll gladly change it to some other behavior. Nikita On Sat, Jun 23, 2012 at 12:50 PM, Sherif Ramadan wrote: > So in other words this patch I submitted should be fine? > > https://github.com/php/php-src/pull/111 > > This particular patch doesn't raise any controversy since it doesn't > touch any of that other stuff and simply puts the defined behavior > back in place as it should have been. --- ## #60947 — Stas Malyshev — 2012-06-23T19:53:01+00:00 URL: https://externals.io/message/60908#60947 · In reply to: #60946 Hi! > The warning for invalid UTF-8 stays intact and is thrown also with > display_errors = On. If this behavior is undesired this can be remedied > later. Must we discuss it 1000 times anew? There was a reason why it didn't throw warning when display_errors is on. The reason is that it is very easy to feed the server configured with display_errors = on wrong JSON and thus force it to reveal information, and this is largely beyond the control of application writer. But what I am worried about even more is getting back to that "commit first, discuss later" mentality. Why we have again a commit in the stable branch before we have a consensus decision what it should be doing? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #60948 — Ferenc Kovacs — 2012-06-23T20:34:26+00:00 URL: https://externals.io/message/60908#60948 · In reply to: #60947 On Sat, Jun 23, 2012 at 9:53 PM, Stas Malyshev wrote: > Hi! > > > […] > > Must we discuss it 1000 times anew? There was a reason why it didn't > throw warning when display_errors is on. The reason is that it is very > easy to feed the server configured with display_errors = on wrong JSON > and thus force it to reveal information, and this is largely beyond the > control of application writer. > > But what I am worried about even more is getting back to that "commit > first, discuss later" mentality. Why we have again a commit in the > stable branch before we have a consensus decision what it should be doing? > from the side, it doesn't seems to be a consensus about having this exception not honoring the display_errors setting. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #60950 — Nikita Popov — 2012-06-23T22:24:56+00:00 URL: https://externals.io/message/60908#60950 · In reply to: #60947 > Hi! > > > […] > > Must we discuss it 1000 times anew? There was a reason why it didn't > throw warning when display_errors is on. The reason is that it is very > easy to feed the server configured with display_errors = on wrong JSON > and thus force it to reveal information, and this is largely beyond the > control of application writer. Yes, I feel like we must discuss this a thousand times anew. There clearly is no consensus about it. Just look at the responses in this thread and in other discussions relating this issue. When people read "throws a warning, but only if display_errors=Off" they automatically replace the Off with an On in their mind. The behavior is simply that unintuitive. Also I'd like to point out that we already decided to *not* use this behavior in other, similar situations. E.g. max_input_vars throws regardless of display_errors (and yes, this was discussed when the feature was introduced). In any case, I don't care *that* much about how json_encode behaves, so I'd be fine with either way. Would just be nice if someone could tell me what that "way" should be. I didn't see any consensus in the discussion. > But what I am worried about even more is getting back to that "commit > first, discuss later" mentality. Why we have again a commit in the > stable branch before we have a consensus decision what it should be doing? I only wanted to sync the branches up. I didn't consider the warning such an important aspect of this issue and thought that it would be okay to simply adjust it in a separate commit. Nikita --- ## #60951 — Gustavo Lopes — 2012-06-23T22:50:13+00:00 URL: https://externals.io/message/60908#60951 · In reply to: #60947 On Sat, 23 Jun 2012 21:53:01 +0200, Stas Malyshev wrote: > Hi! > > > […] > > Must we discuss it 1000 times anew? There was a reason why it didn't > throw warning when display_errors is on. The reason is that it is very > easy to feed the server configured with display_errors = on wrong JSON > and thus force it to reveal information, and this is largely beyond the > control of application writer. I find this argument unconvincing. If `json_encode()` is not supposed to receive badly encoded data, then it's the application writer's responsibility to make sure that the data is correctly encoded. If, on the other hand, you think that's actually not a error on the part of the caller to call `json_encode()` with badly encoded data (i.e., just like `htmlspecialchars()` -- from which I removed the warning removed in 5.4 --, `json_encode()` also accumulates the responsibility validating some aspects of the input), then there's no reason to have a warning at all. In other words, if it's a precondition of `json_encode()` that the input is correctly encoded (and the check is just a form of safe programming), then there's indeed an error on the part of the application writer, if it isn't, then there's no reason for any sort of warning. (By the way, I'd prefer no warning at all, both for this and for inf/NAN. We already can get the error from `json_last_error()` if we want to) -- Gustavo Lopes --- ## #60952 — johannes@schlueters.de — 2012-06-23T23:03:24+00:00 URL: https://externals.io/message/60908#60952 · In reply to: #60950 > Yes, I feel like we must discuss this a thousand times anew. There > clearly is no consensus about it. Just look at the responses in this > thread and in other discussions relating this issue. When people read > "throws a warning, but only if display_errors=Off" they automatically > replace the Off with an On in their mind. The behavior is simply that > unintuitive. Maybe it is simply time to rethink the error handling. The current system doesn't really allow to distinguish between "debugging help" and true "error reporting". I don't have a good proposal, but recently I often struggled whether to throw an `E_NOTICE` from some code in order to help the user while debugging and found myself adding extension-specific error handling routines ... this case here seems to suffer from a related problem. Anybody up for an RFC? (This might also include cleanup the usage of exceptions for internals, and probably converting E_RECOVERABLE to exception) johannes, just throwing some thoughts in the discussion --- ## #60953 — Stas Malyshev — 2012-06-23T23:06:07+00:00 URL: https://externals.io/message/60908#60953 · In reply to: #60952 Hi! > Maybe it is simply time to rethink the error handling. This is true, current error handling has many deficiencies and is also very expensive even when the errors are muted (we still produce the whole error string, etc. even if we never display it). Unfortunately, BC concerns prevailed last times we tried to fix it, but maybe somebody has new ideas on that. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #60954 — Rasmus Lerdorf — 2012-06-24T01:03:34+00:00 URL: https://externals.io/message/60908#60954 · In reply to: #60953 > Hi! > > > […] > > This is true, current error handling has many deficiencies and is also > very expensive even when the errors are muted (we still produce the > whole error string, etc. even if we never display it). Unfortunately, BC > concerns prevailed last times we tried to fix it, but maybe somebody has > new ideas on that. Yes, I think we need to rethink warnings for the cases where the error is part of the common use case. `htmlspecialchars()` getting passed invalid Unicode chars, `json_decode()` getting non-json and `json_encode()` getting non UTF-8 chars. These are all cases where if the function generates an error, in order to avoid that error you have to do some work beforehand to sanitize the input data which is a useless thing to have to do since the functions themselves can do it. For `json_decode()` we have `json_last_error()` to help with debugging. Maybe a solution is to generalize that. We already mostly have it via $php_errormsg/error_get_last(). And yes, I don't think a warning from `json_encode()` is useful because to properly avoid it you would need an `iconv()` call before calling it and that is pure overhead. Short of that your only recourse is to use @ and that just isn't the right answer. In this I would suggest just having it return false and have `json_last_error()` apply to `json_encode()` as well. -Rasmus --- ## #60955 — Nikita Popov — 2012-06-24T12:12:47+00:00 URL: https://externals.io/message/60908#60955 · In reply to: #60951 > If `json_encode()` is not supposed to receive badly encoded data, then it's > the application writer's responsibility to make sure that the data is > correctly encoded. If, on the other hand, you think that's actually not a > error on the part of the caller to call `json_encode()` with badly encoded > data (i.e., just like `htmlspecialchars()` -- from which I removed the warning > removed in 5.4 --, `json_encode()` also accumulates the responsibility > validating some aspects of the input), then there's no reason to have a > warning at all. > > In other words, if it's a precondition of `json_encode()` that the input is > correctly encoded (and the check is just a form of safe programming), then > there's indeed an error on the part of the application writer, if it isn't, > then there's no reason for any sort of warning. > > (By the way, I'd prefer no warning at all, both for this and for inf/NAN. We > already can get the error from `json_last_error()` if we want to) The main problem with not throwing a warning is that it makes debugging the error quite hard. `json_last_error()` only gives you an error code. So you have to use some script like the one used as an example in php.net/json_last_error to figure out what the particular error code actually means. This is obviously a lot more inconvenient than simply seeing "Oh, this failed because I passed an Inf". Nikita --- ## #60957 — Rasmus Lerdorf — 2012-06-24T15:39:48+00:00 URL: https://externals.io/message/60908#60957 · In reply to: #60955 > The main problem with not throwing a warning is that it makes > debugging the error quite hard. `json_last_error()` only gives you an > error code. So you have to use some script like the one used as an > example in php.net/json_last_error to figure out what the particular > error code actually means. This is obviously a lot more inconvenient > than simply seeing "Oh, this failed because I passed an Inf". So how about: mixed json_last_error ( [bool $error_string = false] ) Returns the last error (if any) occurred during the last JSON encoding/decoding. By default an integer constant from the table below is returned. If $error_string is true, an array containing both the error constant and a string representation of the error is returned. --- ## #60970 — Nikita Popov — 2012-06-26T22:48:47+00:00 URL: https://externals.io/message/60908#60970 · In reply to: #60957 > So how about: > > mixed json_last_error ( [bool $error_string = false] ) > > Returns the last error (if any) occurred during the last JSON > encoding/decoding. By default an integer constant from the table below > is returned. If $error_string is true, an array containing both the > error constant and a string representation of the error is returned. Makes sense to me. So should I do this? Remove warnings + add string parameter for json_last_error? Nikita --- ## #60971 — Stas Malyshev — 2012-06-26T23:41:17+00:00 URL: https://externals.io/message/60908#60971 · In reply to: #60970 Hi! > Makes sense to me. So should I do this? Remove warnings + add string > parameter for json_last_error? Looks fine. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #60983 — Benjamin Eberlei — 2012-06-27T11:27:25+00:00 URL: https://externals.io/message/60908#60983 · In reply to: #60971 On Wed, Jun 27, 2012 at 1:41 AM, Stas Malyshev wrote: > Hi! > > > […] > I think its weird that the parameter is called $error_string and setting it to true means returning an array. Why not in the spirit of others have a new function `json_last_error_msg()` or something similar? > > Looks fine. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 --- Next page: https://externals.io/message/60908.md?page=2