contao / check

Contao Check
GNU Lesser General Public License v3.0
47 stars 23 forks source link

Wrong if condition handlingin controller/boostrap.php #93

Closed tniebergall closed 8 years ago

tniebergall commented 8 years ago

I think the following code block is wrong, because the function is triggered when it doesn't exist and causes an server error 500.

// Add the posix_getpwuid function
if (!function_exists('posix_getpwuid')) {
    function posix_getpwuid($int) {
        return array('name'=>$int);
    }
}

It should be

// Add the posix_getpwuid function
if (function_exists('posix_getpwuid')) {
    function posix_getpwuid($int) {
        return array('name'=>$int);
    }
}
leofeyer commented 8 years ago

I don't think so. The function must only be defined if it does not yet exist, mustn't it?

tniebergall commented 8 years ago

Yes you're right that was my mistake.

But if you execute the original code on a system where posix_getpwuid isn't enabled, you get the described server error 500.

Whit the changed code the error wont appear.

leofeyer commented 8 years ago

What is written into the server log file when the error occurs?

tniebergall commented 8 years ago

PHP Fatal error: Cannot redeclare posix_getpwuid() in /var/www/web204/htdocs/contao-3-5/check/controller/bootstrap.php on line 173

fritzmg commented 8 years ago

This error has been around for a long time. See https://community.contao.org/de/showthread.php?8940-System-Check-Fatal-error-posix_getpwuid&p=76537&viewfull=1#post76537 for one possible workaround for example.

leofeyer commented 8 years ago

Your "workaround" is not really a workaround. You might as well remove the function declaration completely, because create_function() creates an anonymous function and not one called posix_getpwuid (). Are you sure this is not a duplicate of #86?

fritzmg commented 8 years ago

Well it's not 'my' workaround - but the issue has been around for that long. Yes, the issue is probably a duplicate of #86 - however, that issue was closed by the reporter.

leofeyer commented 8 years ago

Correct, because a PHP update apparently solved the issue. (Which would indicate a PHP bug.)

fritzmg commented 8 years ago

Indeed. However, I have no idea under which conditions (specific PHP version, specific PHP build of specific Linux distris) this issue might be caused. It just happens to pop up every now and again on various hosters/systems.

qzminski commented 8 years ago

It can also happen if the function is disabled for security reasons, see error:

Warning: posix_getpwuid() has been disabled for security reasons in /home/mirand/domains/domain.com/public_html/check/controller/file-permissions.php on line 142 

Then the function_exists() seem to return false but the function name itself is already registered.

leofeyer commented 8 years ago

So what's the status on this issue?

tniebergall commented 8 years ago

So for my opinion it could be closed. I solved it for the corresponding server, but this was the first and only one with this error.

fritzmg commented 8 years ago

If the only cause of this issue is posix_getpwuid being disabled via disable_functions, then a possible "fix" for the check might be:

if (!function_exists('posix_getpwuid') && !in_array('posix_getpwuid', explode(',', ini_get('disable_functions')))) {
    function posix_getpwuid($int) {
        return array('name'=>$int);
    }
}

Then you'll get at least a more useful warning:

screen shot 2016-09-12 at 09 59 12

instead of

PHP Fatal error:  Cannot redeclare posix_getpwuid() in …
leofeyer commented 8 years ago

I assume you have copied this "solution" from the PHP manual? Did you realize that both these answers have been downvoted?

This is actually a PHP bug, which has been fixed already: https://bugs.php.net/bug.php?id=69297&edit=3

fritzmg commented 8 years ago

No, I did not copy that from anywhere. My "solution" simply prevents the redeclaration of posix_getpwuid when it is defined in disable_functions.

The PHP bug you referred to is something else entirely and has nothing to do with this issue.

fritzmg commented 8 years ago

The problem occurs under PHP 5.4.39, 5.6.24 and 7.0.9.

As long as posix_getpwuid is defined in disable_functions, the Contao Check will run into the aforementioned fatal error.

leofeyer commented 8 years ago

My "solution" simply prevents the redeclaration of posix_getpwuid when it is defined in disable_functions.

Which does not improve anything at all! You are just exchanging one error message with another.

fritzmg commented 8 years ago

I know, but, as I said, at least then you are getting a warning telling you what the actual problem is. Of course you could also simply do

if (in_array('posix_getpwuid', explode(',', ini_get('disable_functions'))))
{
    die("'posix_getpwuid' must not be in 'disable_functions'");
}

or something better.

In any case, the fatal error should be avoided somehow.

fritzmg commented 8 years ago

The Contao Check is supposed to be able to run on most configurations without generating a Fatal Error (or die for that matter…) - since it is used to determine whether the respective environment meets the minimum requirements for Contao amongst other things (like installing Contao and validating an existing installation).

Thus, instead of generating a fatal error (current situation), a warning or just a die(), maybe the disable_functions check could be done inside file-permissions.php or wherever, i.e. where the posix_getpwuid function is needed. This way the Contao Check will generally work and it can tell you that posix_getpwuid is disabled and thus the Contao Check cannot test the file permissions.

This way, a regular User using the Contao Check will more likely know what to do.