On Thu, Jun 25, 2015 at 3:55 PM, Johannes Schlüter <johan...@schlueters.de> wrote:
> On Thu, 2015-06-25 at 14:52 +0200, Nikita Popov wrote: > > However what it doesn't do, and what I wouldn't consider feasible to do, > is > > ensure that every single string conversion in library functions is > > exception safe. Personally I don't think this is a blocking issue, as the > > worst that can happen is usually an additional superfluous warning to be > > thrown, or something similar. If cases like this turn up, we can > > specifically target them. > > I don't agree to the assesment that this isn'T a problem. Consider this > extension pseudo-code: > > > zval *data = get_data(); > convert_to_string(data); > store_into_database(Z_STRVAL_P(data)); > return TRUE; > > This will store wrong data in the database and report to the user that > there was an error before storing, so the user assumes nothing was > stored. > I can see your concern here -- however this is nothing specific to exceptions thrown from __toString(). There is are a number of ways you can end up in this situation, the two most common being: a) Error handlers can (and often do) throw. E.g. it is possible to convert the two recoverable fatal errors that __toString() currently throws into exceptions and end up in the situation you describe, only with even less safety and more leaks. However this applies to all occurrences of zend_error or php_error_docref that generate a non-fatal error. From a quick grep I estimate we have about 4000 places in the codebase where this can occur and I'm sure many of them do not check for EG(exception) immediately after throwing the error. b) Destructors can throw. We have approximately 2500 places in php-src destroying a zval, which could potentially throw. I don't think I've ever seen an EG(exception) check after a zval_ptr_dtor(). Given that we rely on best-effort exception handling in so many cases, it's not clear to me why exceptions thrown from __toString() should be expressly forbidden. If you want to protect against side-effects after exceptions in critical places like database interaction, we can introduce exception checks right before the operation is executed and prevent anything bad from happening regardless of where the exception came from. But as said, I think this is only tangential to the question of __toString(), as this is a general issue. Nikita
