[dev] [commits] Horde branch master updated. e2bb4297227910132c9cdc90dfa7af73e5def8db

[Michael M Slusarz]

Quoting Michael M Slusarz <slusarz at horde.org>:

> Quoting Jan Schneider <jan at horde.org>:
>
>> Zitat von Michael M Slusarz <slusarz at horde.org>:
>>
>>> Quoting Michael J Rubinsky <mrubinsk at horde.org>:
>>>
>>>> Quoting Michael M Slusarz <slusarz at horde.org>:
>>>>
>>>>> commit 48f26df17fd8b36e37f732b223d17dd7bd10b68c
>>>>> Author: Michael M Slusarz <slusarz at horde.org>
>>>>> Date:   Wed Jan 9 15:20:43 2013 -0700
>>>>>
>>>>> [mms] Remove call-time pass-by-reference, which was causing PAM  
>>>>> authentication driver to fail in PHP 5.4+.
>>>>>
>>>>> framework/Auth/lib/Horde/Auth/Pam.php |    2 +-
>>>>> framework/Auth/package.xml            |    4 ++--
>>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> http://git.horde.org/horde-git/-/commit/48f26df17fd8b36e37f732b223d17dd7bd10b68c
>>>>
>>>> At one time this was required to avoid some weird fatal error,  
>>>> though I don't remember if it was from pam or (the now removed)  
>>>> pam_auth.
>>>
>>> If this is the case, this needs to be immediately removed from  
>>> framework.  The pass-by-reference is a FATAL error in PHP 5.4, and  
>>> can't be worked around via conditionals.  And it can't live in the  
>>> framework tree either, or else we have to remove our PHP  
>>> error-detection on a git commit (this file crashes PHP 5.4 and  
>>> won't allow a commit to succeed).
>>
>> Ah, PAM again, like every 6 months or so. So here's the history:
>> - First there was the PAM extension from PECL which was abandoned quickly
>> - A fork was created that made it into Debian
>> - The PECL version was picked up again, maintained and kept up to date.
>> - The two versions diverged and we had a switch in our PAM driver  
>> to support both.
>> - At one point we decided to drop support for the Debian version  
>> because it was only available there and no longer actively  
>> developed, unlike the PECL version.
>> - Through all this time, the PECL version required  
>> pass-by-reference to work.
>> - Today, the PECL version seems abandoned again, the Debian version  
>> is still only available there (state unknown), has even been remove  
>> from Ubuntu.
>> - Yet it's still the only way to authenticate with system users,  
>> and we use it for Horde internally too.
>>
>> We could simply skip the error parameter to work around this. We  
>> would lose detailed error messages, but at least we won't have to  
>> drop the driver completely.
>
> It's either that or remove completely from code base.  Or someone  
> could provide a patch to fix the pass-by-reference in PECL, and then  
> we require a minimum version of the module.  Sigh... I guess I will  
> give my best 15 minute shot at fixing in the C source.

Upon further inspection... this SHOULD work just fine.  The PECL pam  
code isn't doing anything different than, say, stream_socket_client()  
which has a function definition of:

resource stream_socket_client ( string $remote_socket [, int &$errno  
[, string &$errstr [, float $timeout =  
ini_get("default_socket_timeout") [, int $flags =  
STREAM_CLIENT_CONNECT [, resource $context ]]]]] )

As the doc example shows, there is no need to pass call-by-reference:

$fp = stream_socket_client("tcp://www.example.com:80", $errno, $errstr, 30);


Within the C source, stream_socket_clients() arguments are parsed by:

zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|zzdlr", &host,  
&host_len, &zerrno, &zerrstr, &timeout, &flags, &zcontext)

The key thing to point out is the "s|zzdlr".  This defines the list of  
parameters.  For our purposes (analyzing the $errstr parameter), what  
is important is that this parameter is defined as 'z', meaning a  
direct access to the underlying Zend variable.  It is linked to  
&zerrstr by zend_parse_parameters().  Later in that method, zerrstr is  
(potentially) set via:

         zval_dtor(zerrstr);
         ZVAL_STRING(zerrstr, "", 1);

I'm going to assume this code destroys the current value of zerrst and  
then sets it to a new value ('looks like the empty string) via the  
ZVAL_STRING function/macro.


So looking at the pam_auth() code, it is parsing parameters via:

zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ss|zb", &username,  
&username_len, &password, &password_len, &status, &checkacctmgmt)

So &status is a pointer to the variable in PHP userland.  This  
variable is modified by:

             spprintf(&error_msg, 0, "%s (in %s)", (char *)  
pam_strerror(pamh, result), "pam_start");
             zval_dtor(status);
             ZVAL_STRING(status, error_msg, 0);

In other words... identical to socket_stream_client().


Thus, removing the call-time pass-by-reference is appropriate and  
doesn't break anything (at least with the PECL PAM extension).  So we  
should be good with the change.

And now my head hurts.  C?  Bleh.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]