# Allow throwing from __toString() - Thread: https://externals.io/message/104535 - Original archive: https://news-web.php.net/php.internals/104535 - Started by Nikita Popov on 2019-03-01T11:25:47+00:00 - Messages: 6 - 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 --- ## #104535 — Nikita Popov — 2019-03-01T11:25:47+00:00 URL: https://externals.io/message/104535#104535 Hi internals, We currently forbid throwing exceptions from __toString() and instead convert them into fatal errors. The rationale for this behavior is that string conversions are performed in many places throughout the engine and standard library, and not all places performing these conversions will deal with exceptions correctly (in the sense of "as soon as possible"). While this limitation is ultimately futile, because exceptions during string conversion can still be triggered by error handlers that convert warnings to exceptions, the sentiment in past discussions on this topic has been to not relax this restriction until we have audited string conversions across the codebase for exception safety. I have now done this, with the patch available at https://github.com/php/php-src/pull/3887. The PR * allows exceptions to be thrown from __toString(). * converts the "could not be converted to string" and "__toString() must return a string value" recoverable fatal errors into proper Error exceptions. * audits uses of zval_get_string(), convert_to_string() and friends across the codebase and adds exception checks where necessary. For extension authors, the guideline is: * If zval_get_string() / convert_to_string() generate an exception, they will always produce an interned string. This means that the result does not need to be freed in case of an exception (but freeing it is also fine -- do whatever is more convenient). * The return value will be an empty string if an object to string conversion fails, and "Array" if an array is converted to string and the resulting notice is promoted to an exception by an error handler. (This behavior is as before.) * As usual, whether an exception has been thrown may be checked using if (EG(exception)). A typical example would be: zend_string *str = zval_get_string(val); if (EG(exception)) { return; // Possibly do some cleanup here. } * While checking every single string conversion certainly puts you on the safe side, leaving out these checks will usually only result in some unneeded computation and possibly redundant warnings. The main thing you should watch out for are operations modifying persistent structures such as databases. I would like to land this change in PHP 7.4. Are there any objections to that? I'm skipping an RFC for this, because the change here is pretty much a purely technical matter, but I can put up an RFC if it turns out to be controversial. Regards, Nikita PS: If I remember correctly https://wiki.php.net/rfc/array-to-string was accepted but never implemented due to the inability to throw from string conversions. We should probably revisit this for PHP 8.0. --- ## #104539 — johannes@schlueters.de — 2019-03-01T16:24:19+00:00 URL: https://externals.io/message/104535#104539 > For extension authors, the guideline is: Will zend_parse_paramters and related detect if an exception is thrown and fail? I believe things like database (or other network) extensions have to be really carefully checked, not that we store corrupted data (empty string) in the database (or otherwise send via network) while returning an error to the user. Simple 5 minute example based on your branch: exec('CREATE TABLE t(id int, v varchar(255))'); $stmt = $db->prepare('INSERT INTO t VALUES(:i, :v)'); $stmt->bindValue('i', 1234); $stmt->bindValue('v', new throws); try {   $stmt->execute(); } catch (Exception $e) {   echo "Exception thrown ...\n"; } $stmt->execute(); $query = $db->query("SELECT * FROM t"); while ($row = $query->fetchArray(SQLITE3_ASSOC)) { print_r($row); } ?> This prints Exception thrown ... Array (     [id] => 1234     [v] =>  ) So during the first execution it notices that the conversion went wrong and aborts the operation, but it keeps th emtpy string as bound value. On second execute it re-uses the values and doesn't notice the error. I fear we have many such cases which are subtle ad hard to find without deep review of any string conversion. And in future we will introduce bugs due to this in places where new conversions are added ... johannes --- ## #104540 — Nikita Popov — 2019-03-01T17:28:44+00:00 URL: https://externals.io/message/104535#104540 On Fri, Mar 1, 2019 at 5:24 PM Johannes Schlüter wrote: > > > > […] > > Will zend_parse_paramters and related detect if an exception is thrown > and fail? > Yes, of course :) That's one of the most important cases. I believe things like database (or other network) extensions have to be > really carefully checked, not that we store corrupted data (empty > string) in the database (or otherwise send via network) while returning > an error to the user. > > > Simple 5 minute example based on your branch: > > > class throws { > function __toString() { > throw new Exception("Sorry"); > } > } > > $db = new sqlite3(':memory:'); > $db->exec('CREATE TABLE t(id int, v varchar(255))'); > > $stmt = $db->prepare('INSERT INTO t VALUES(:i, :v)'); > $stmt->bindValue('i', 1234); > $stmt->bindValue('v', new throws); > > try { > $stmt->execute(); > } catch (Exception $e) { > echo "Exception thrown ...\n"; > } > > $stmt->execute(); > > $query = $db->query("SELECT * FROM t"); > while ($row = $query->fetchArray(SQLITE3_ASSOC)) { > print_r($row); > } > ?> > > This prints > > Exception thrown ... > Array > ( > [id] => 1234 > [v] => > ) > > So during the first execution it notices that the conversion went wrong > and aborts the operation, but it keeps th emtpy string as bound value. > On second execute it re-uses the values and doesn't notice the error. > Thanks for the example. I agree that keeping the empty value is not the correct behavior and have fixed this in https://github.com/php/php-src/pull/3887/commits/ac14ab02912b267dd370396937a971ff0b4daec8. I will also review the other database binding code, because I think I made the same mistake there. So there's one more point to add to the extension guidelines: * When working on non-temporary zvals, avoid leaving behind the converted string in case of exception. Prefer zend_string *str = zval_get_string(val); if (EG(exception)) { return; } // Do something with str zend_string_release(str); over convert_to_string(val); if (EG(exception)) { return; } // Do something with Z_STR_P(val) Of course, this should be preferred for other reasons as well. While working on this patch I've already fixed many illegal uses of convert_to_string, which were modifying shared values in-place. > I fear we have many such cases which are subtle ad hard to find without > deep review of any string conversion. And in future we will introduce > bugs due to this in places where new conversions are added ... I'm happy to find and fix these bugs, rather than leave the language buggy by design ;) Regards, Nikita On Fri, Mar 1, 2019 at 5:24 PM Johannes Schlüter wrote: > > > > […] > > Will zend_parse_paramters and related detect if an exception is thrown > and fail? > > I believe things like database (or other network) extensions have to be > really carefully checked, not that we store corrupted data (empty > string) in the database (or otherwise send via network) while returning > an error to the user. > > > Simple 5 minute example based on your branch: > > > class throws { > function __toString() { > throw new Exception("Sorry"); > } > } > > $db = new sqlite3(':memory:'); > $db->exec('CREATE TABLE t(id int, v varchar(255))'); > > $stmt = $db->prepare('INSERT INTO t VALUES(:i, :v)'); > $stmt->bindValue('i', 1234); > $stmt->bindValue('v', new throws); > > try { > $stmt->execute(); > } catch (Exception $e) { > echo "Exception thrown ...\n"; > } > > $stmt->execute(); > > $query = $db->query("SELECT * FROM t"); > while ($row = $query->fetchArray(SQLITE3_ASSOC)) { > print_r($row); > } > ?> > > This prints > > Exception thrown ... > Array > ( > [id] => 1234 > [v] => > ) > > So during the first execution it notices that the conversion went wrong > and aborts the operation, but it keeps th emtpy string as bound value. > On second execute it re-uses the values and doesn't notice the error. > > I fear we have many such cases which are subtle ad hard to find without > deep review of any string conversion. And in future we will introduce > bugs due to this in places where new conversions are added ... > > johannes --- ## #104541 — johannes@schlueters.de — 2019-03-01T20:31:26+00:00 URL: https://externals.io/message/104535#104541 > made the same mistake there. So there's one more point to add to the > extension guidelines: Do we have a persistent space to keep those? The internals manual is limited ... :-) > I'm happy to find and fix these bugs, rather than leave the language > buggy by design ;) The "old man" in me is skeptical, especially since literally the second program variation I tried broke in a really subtle way which is hard to debug and understand for users. Also I claim that if a string conversion throws an error, it's probably no good idea to do that conversion in an implicit magic function. (but yeah, that architectural choice doesn't necessarily have to be a restriction in the language) The third thing I tried was around `sort()`, where we don't have exception guarantees specified and work on a reference: results in Array (     [0] => a     [1] => b     [2] => throws Object         (         ) ) Thus data is modified, while an exception is thrown. But I believe this case isn't bad, while I assume there might be other cases, where the effect has impact. (references should be avoided in idiomatic PHP anyways ...) Anyways, to help more productively with your young spirit: Years back, when we changed string length handling, I created a clang- analyzer plugin, which checks zend_parse_paramters() calls for the right type. https://github.com/johannes/clang-php-checker I assume it should be possible to create a clang-analyz (or clang-tidy) check which ensures that conversions are done properly and are checking for exceptions. This might help third-party extension authors (and if integrated with the build also long-term ...) with such requirements. johannes --- ## #104542 — Christoph M. Becker — 2019-03-01T21:52:02+00:00 URL: https://externals.io/message/104535#104542 > > > […] > > Do we have a persistent space to keep those? The internals manual is > limited ... :-) And should be removed altogether, in my opinion[1]. Such info seems to be more suitable for , or another place where more people experienced with the engine are involved. [1] ff -- Christoph M. Becker --- ## #104543 — johannes@schlueters.de — 2019-03-01T22:06:21+00:00 URL: https://externals.io/message/104535#104543 > > > > […] > >And should be removed altogether, in my opinion[1]. Such info seems to >be more suitable for , or another >place where more people experienced with the engine are involved. I would prefer if we could get the authors of that site to contribute it to PHP.net (be it in the manual or some other place) so that a) it receives the official stamp b) receives visibility by extension authors c) receives more participation by the community But that's a different topic than the proposed patch :) johannes >[1] ff