nette / latte

☕ Latte: the safest & truly intuitive templates for PHP. Engine for those who want the most secure PHP sites.
https://latte.nette.org
Other
1.14k stars 109 forks source link

Throwing an exception in acquireLock(...) fails #265

Closed sukovf closed 3 years ago

sukovf commented 3 years ago

Version: 2.10 PHP: 8.0.3

Bug Description

Throwing an exception at Engine.php:247 fails as Latte expects the function error_get_last() to always return an array from what it tries to read the message key. However, the aforementioned function can also return NULL, see https://www.php.net/manual/en/function.error-get-last.php

throw new RuntimeException("Unable to create directory '$dir'. " . error_get_last()['message']);

Steps To Reproduce

set_error_handler('myErrorHandler');// <- this is what makes the error_get_last() func to always return NULL instead of an array
$latte = new Latte\Engine();
$latte->setTempDirectory('temp');// <- make sure your app can't access or make that dir!!!
$output = $latte->renderToString('random_template.latte');
dg commented 3 years ago

I expect if mkdir() returns false, then error_get_last() will return last error. Under what circumstances does it fail?

sukovf commented 3 years ago

It's a very simple Slim2-based app but I see the problem now: If you set a custom error handler through the set_error_handler(...) function (Slim2 does that by default), then the error_get_last() function always returns NULL.

EDIT: I've updated the Steps To Reproduce section in my original post.

dakujem commented 3 years ago

👉 @ Error Suppression operator does not silent fatal errors

This dirty draft fixes it for me.

    private function isExpired(string $file, string $name): bool
    {
        try {
            return $this->autoRefresh && $this->getLoader()->isExpired($name, (int)@filemtime($file)); // @ - file may not exist
        } catch (ErrorException $e) {
            return $this->getLoader()->isExpired($name, 0);
        }
    }

Please fix asap.

PHP 8.0.3 , built-in server, Windows.

dakujem commented 3 years ago

I have to add that in my project setup, all errors, notices and warnings are turned into an ErrorException. I'm not completely sure if this is anyhow related or not.

dg commented 3 years ago

@dakujem yes, it is related, simply don't do it. And your issue is completely unrelated to @sukovf issue and to "Error Suppression operator does not silent fatal errors".

@sukovf custom error handler should returns false to fill-in error_get_last()

sukovf commented 3 years ago

@dg I'm not sure if I understand what you mean. The custom error handling function in Slim2 doesn't return anything, it just throws an exception built upon the data of catched PHP errors. Yes I can override that error handler but I think it's generally not a good idea to treat the output of a function directly as an array while it can also return NULL.

EDIT: Hmm whenever an exception is thrown, the code execution should stop and never reach the point when the func error_get_last() is called. Weird...

mabar commented 3 years ago

error_get_last() returns null only in case of no error and when the error is removed by error_clear_last() or by (incorrectly implemented) error handler. It's generally reliable behavior with exception of some functions which can fail without creating an error (like mail()). In this case, it's error handler failure - it should return false, because it didn't handle error.

dg commented 3 years ago

@sukovf in Slim2 error handler is bug, there https://github.com/slimphp/Slim/blob/7d3308bb8b41238941c0cbf549ef103c1bd69a30/Slim/Slim.php#L1407 should be return false. Without false it completely disables the function error_get_last().

It's weird PHP behavior, but there's nothing we can do about it.

https://phpfashion.com/how-to-write-error-handler-in-php

sukovf commented 3 years ago

@dg Yep I see it now thanks to @mabar (y). It's time to finally convince my colleagues to upgrade. Thanks for your time.

dg commented 3 years ago

Or you can set your custom and fixed error handler.

dakujem commented 3 years ago

@sukovf to clarify the previous answers, you only need to return false instead of return; (null) at the line David posted above. The most trivial and striaght-forward way to do this would be by extending the Slim\Slim class to override the handleErrors method.

// for Slim v2
class MySlimApp extends \Slim\Slim
{
    public static function handleErrors($errno, $errstr = '', $errfile = '', $errline = '')
    {
        if (!($errno & error_reporting())) {
            return false; // <--- Note the returned false here
        }
        throw new \ErrorException($errstr, $errno, 0, $errfile, $errline);
    }
}
dakujem commented 3 years ago

@dg The issue I'm facing is not completely unrelated. It's related to error suppression using @ which temporarily changes the error-reporting level. Given an incorrect error handler, this breaks Latte (and undoubtfully other code using @).

I was using error handler for PHP 7, which stopped working after the upgrade to PHP 8. I'm including this for sake of other people who stumble upon the same/similar issue.

set_error_handler(function ($errno, $errstr, $errfile, $errline) {
    // WARNING: DO NOT DO THIS kind of comparison, it fails in PHP 8+
    if (0 === error_reporting()) {
        // error was suppressed with the @-operator (only works until PHP 7.4)
        return false;
    }
    throw new ErrorException($errstr, 0, $errno, $errfile, $errline);
});

I believe this snippet comes from some older Slim tutorial or similar resource.

dg commented 3 years ago

I wrote it down here https://phpfashion.com/how-to-write-error-handler-in-php