Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:104540
MIME-Version: 1.0
References: <CAF+90c9XDkT2k4jGy+z++5Z67ga=Z-DjR3wjMdLp7zPaFcX5AQ@mail.gmail.com>
 <1551457459.2096.19.camel@schlueters.de>
In-Reply-To: <1551457459.2096.19.camel@schlueters.de>
Date: Fri, 1 Mar 2019 18:28:44 +0100
Message-ID: <CAF+90c-0wfuvYZoGYLv82g8OepshCdHQ2XPYyGdSp6C3vg7wGg@mail.gmail.com>
To: =?UTF-8?Q?Johannes_Schl=C3=BCter?= <johannes@schlueters.de>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001f60a905830bbe56"
Subject: Re: [PHP-DEV] Allow throwing from __toString()
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000001f60a905830bbe56
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 1, 2019 at 5:24 PM Johannes Schl=C3=BCter <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 =3D new sqlite3(':memory:');
> $db->exec('CREATE TABLE t(id int, v varchar(255))');
>
> $stmt =3D $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 =3D $db->query("SELECT * FROM t");
> while ($row =3D $query->fetchArray(SQLITE3_ASSOC)) {
>         print_r($row);
> }
> ?>
>
> This prints
>
>     Exception thrown ...
>     Array
>     (
>         [id] =3D> 1234
>         [v] =3D>
>     )
>
> 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/ac14ab02912b267dd370396937=
a971ff0b4daec8.
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 =3D 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=C3=BCter <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?
>
> 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 =3D new sqlite3(':memory:');
> $db->exec('CREATE TABLE t(id int, v varchar(255))');
>
> $stmt =3D $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 =3D $db->query("SELECT * FROM t");
> while ($row =3D $query->fetchArray(SQLITE3_ASSOC)) {
>         print_r($row);
> }
> ?>
>
> This prints
>
>     Exception thrown ...
>     Array
>     (
>         [id] =3D> 1234
>         [v] =3D>
>     )
>
> 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
>

--0000000000001f60a905830bbe56--