Everything

php

Allow throwing from __toString()

php.internals

Nikita Popov

7 years ago

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.

Johannes Schlueter

7 years ago

On Fr, 2019-03-01 at 12:25 +0100, Nikita Popov 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

Nikita Popov

7 years ago

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

> On Fr, 2019-03-01 at 12:25 +0100, Nikita Popov 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:

Johannes Schlueter

7 years ago

On Fr, 2019-03-01 at 18:28 +0100, Nikita Popov wrote:

> 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

Christoph Becker

7 years ago

On 01.03.2019 at 21:31, Johannes Schlüter wrote:

> On Fr, 2019-03-01 at 18:28 +0100, Nikita Popov wrote:
>> 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/969386904>ff

--
Christoph M. Becker

Johannes Schlueter

7 years ago

On March 1, 2019 10:52:02 PM GMT+01:00, "Christoph M. Becker" <cmbecker69@gmx.de> wrote:

>On 01.03.2019 at 21:31, Johannes Schlüter wrote:
>
>> On Fr, 2019-03-01 at 18:28 +0100, Nikita Popov wrote:
>>> 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