Re: [PHP-DEV] Allow throwing from __toString()

[Johannes Schlüter]

On March 1, 2019 10:52:02 PM GMT+01:00, "Christoph M. Becker" 
 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 , 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Allow throwing from __toString()

[Christoph M. Becker]

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 , or another
place where more people experienced with the engine are involved.

[1] ff

-- 
Christoph M. Becker

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Allow throwing from __toString()

[Johannes Schlüter]

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:



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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Allow throwing from __toString()

[Nikita Popov]

On Fri, Mar 1, 2019 at 5:24 PM Johannes Schlüter 
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:
>
> 
> 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:

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

Re: [PHP-DEV] Allow throwing from __toString()

[Johannes Schlüter]

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:

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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php