Allow throwing from __toString() View as Markdown

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.

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:

<?php

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

On Fri, Mar 1, 2019 at 5:24 PM Johannes Schlüter johannes@schlueters.de
wrote:

For extension authors, the guideline is:

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:

<?php

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 johannes@schlueters.de
wrote:

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:

<?php

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

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:

<?php
$a = [
  'a',
  new throws,
  'b',
];

try {
  sort($a);
} catch (Exception $e) {
}
print_r($a);
?>

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

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 ... :-)

And should be removed altogether, in my opinion[1]. Such info seems to
be more suitable for http://www.phpinternalsbook.com/, or another
place where more people experienced with the engine are involved.

[1] http://news.php.net/php.doc/969386904ff

--
Christoph M. Becker

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 ... :-)

And should be removed altogether, in my opinion[1]. Such info seems to
be more suitable for http://www.phpinternalsbook.com/, 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] http://news.php.net/php.doc/969386904ff