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