Last modified: 2010-05-15 16:03:29 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T16178, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 14178 - Some uses of UserLoadFromSession hook cause segfault
Some uses of UserLoadFromSession hook cause segfault
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.13.x
All All
: Normal critical (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-19 05:15 UTC by Craig
Modified: 2010-05-15 16:03 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
New hook called after loadFromSession is called (513 bytes, patch)
2008-10-13 18:53 UTC, Ryan Lane
Details

Description Craig 2008-05-19 05:15:03 UTC
Using the UserLoadFromSession hook causes PHP to segfault. The line that actually segfault is the one with "call_user_func_array" in includes/Hooks.php

The call to run the hooks for UserLoadFromSession is on line 771 in includes/User.php - I believe the problem is in the parameters passed to wfRunHooks on that line:
wfRunHooks( 'UserLoadFromSession', array( $this, &$result ) );

Here's a simple case to reproduce the segfault:
$wgHooks['UserLoadFromSession'][] = 'testFn';

function testFn(&$user){

}
Comment 1 Aaron Schulz 2008-05-19 05:18:48 UTC
works for me
Comment 2 Craig 2008-05-19 05:37:55 UTC
I oversimplified the test case:

$wgHooks['UserLoadFromSession'][] = 'testFn';

function testFn(&$user){
        //Give us a user, see if we're around
        $tmpuser = User::newFromSession();

        //They already with us?  If so, quit this function.
        if( $tmpuser->isLoggedIn() ) {
                $wgAuth->printDebug( "User is already logged in.", 1 );
                return true;
        }
        // more login actions...
}

After some more debugging, I learned that UserLoadFromSession isn't a simple rename of the AutoAuthenticate hook - if you call User::newFromSession() from within a UserLoadFromSession hook, an infinite loop results. It seems some more documentation or something may be needed :-)

How do I determine if a user is already logged in from within the UserLoadFromSession hook function?
Comment 3 Aaron Schulz 2008-05-19 05:41:05 UTC
Yeah, I see, that code breaks
Comment 4 Aaron Schulz 2008-05-19 05:43:03 UTC
Still, I don't see why you need to be loading another user.
Comment 5 Craig 2008-05-19 05:46:03 UTC
I just want to figure out if the user is already logged in, (perhaps he was logged in via another method, like cookies), so I can avoid logging them in again (which is expensive).

Is there a way to do that without using the method I previously described? Calling "$user->isLoggedIn()" inside testFn always returns false.
Comment 6 Aaron Schulz 2008-05-19 05:50:38 UTC
$user->isLoggedIn() should not fail if $user is the object passed from the hook.  This is because $user->load() shortcircuits since it was already loaded. Calling most methods on User triggers $user->load(). When you make a new user object, it isn't already loaded, so $newuser->load() calls the session function and hook, which causes recursion.
Comment 7 Craig 2008-05-19 05:57:43 UTC
$wgHooks['UserLoadFromSession'][] = 'testFn';

function testFn(&$user){
   if( $user->isLoggedIn() ) {
     die("Logged in");
   }
}

Even after logging in, that "die" will never execute. From my reading of your last statement, it sounds like after logging in, it should execute.... right?
Comment 8 Aaron Schulz 2008-05-19 06:06:57 UTC
You can run the code from comment #2, with $tmpuser replaced with $user
Comment 9 Aaron Schulz 2008-05-19 06:09:01 UTC
(In reply to comment #8)
> You can run the code from comment #2, with $tmpuser replaced with $user
> 

Well not exactly. As long as User::newFromSession() is not called of the user is already loaded.
Comment 10 Ryan Lane 2008-08-15 23:03:39 UTC
(In reply to comment #6)
> $user->isLoggedIn() should not fail if $user is the object passed from the
> hook.  This is because $user->load() shortcircuits since it was already loaded.
> Calling most methods on User triggers $user->load(). When you make a new user
> object, it isn't already loaded, so $newuser->load() calls the session function
> and hook, which causes recursion.
> 

$user-isLogedIn() will always return false in this situation.

The UserLoadFromSession hook is called before the user is loaded from the session. As such, the hook is always going to pass an anonymous user.

A workaround for this problem in the past (when the hook was still AutoAuthenticate), was to create a temporary user, call User::loadFromSession, and then check if that user was logged in. There is no way of doing this now.

Auto-auth plugins now either have to reimplement User::loadFromSession in their plugin, or re-authenticate users on every page view. I've chosen to do the former.

My suggested solution is to have User::loadFromSession call the UserLoadFromSession hook, then call another function (User::loadFromSessionBody?) that isn't marked as private to do the rest. That way auto-auth plugins can call $user->loadFromSessionBody(), *then* call $user->isLoggedIn().
Comment 11 Aaron Schulz 2008-10-13 04:51:02 UTC
Looking at this hook, the above uses just seem to be beyond what it was intended for, so I'm not sure it's worth changing. If a new hook is needed, then that can be filed as a another bug report.
Comment 12 Ryan Lane 2008-10-13 17:58:39 UTC
Maybe you don't realize this, but this wasn't originally the hook used for this. The hook originally used was AutoAuthenticate, and it was renamed to this, and moved to this spot.

This hook is fully usable for auto-authentication, after all, the CentralAuth plugin uses it. The only issue with it, is that auto-authentication plugins can't check to see if a user has been logged in or not. The solution I offered gives the auto-authentication plugins a way to check this (the way they previously were).

I don't necessarily have an issue with making a new hook, but we need a stable api for auto-authentication; we can't keep changing how it works every release.

If you'd like to fix the problem, please fix it permanently this time.
Comment 13 Aaron Schulz 2008-10-13 18:04:34 UTC
Obviously it is used for authentication, but it is there to fetch the cookie data not to call another loadFromSession(), which is just recursive.
Comment 14 Ryan Lane 2008-10-13 18:08:36 UTC
There is a difference between MediaWiki core loading something from session, and from an extension doing it via a hook. Some plugins would like to allow core to do session checking for them.

I don't mind if another hook is made, but as it is a solution to this problem, let's leave the bug open until we actually fix the problem.
Comment 15 Aaron Schulz 2008-10-13 18:16:22 UTC
(In reply to comment #14)
> There is a difference between MediaWiki core loading something from session,
> and from an extension doing it via a hook. Some plugins would like to allow
> core to do session checking for them.
> 
> I don't mind if another hook is made, but as it is a solution to this problem,
> let's leave the bug open until we actually fix the problem.
> 

The problem is assumed to have to do with the bug summary and initial report. Unless the fix is to stop it from segfaulting, then this bug is a wontfix. Adding a new hook could solve the issue of authenticating by checking for an existing initialized session first, but not the issue of segfaults.

I suppose one could change the bug summary...
Comment 16 Ryan Lane 2008-10-13 18:53:06 UTC
Created attachment 5424 [details]
New hook called after loadFromSession is called

Here's a patch to make a new hook (UserLoadAfterLoadFromSession), which is called in load, after loadFromSession() is called.

Using this hook, auto-authentication plugins won't need to call $user->loadFromSession(), and will instead just be able to check $user->isLoggedIn().
Comment 17 Ryan Lane 2008-10-13 19:03:58 UTC
auto-authentication plugins that wish to have core load the user from session should use the UserLoadAfterLoadSession hook now instead of UserLoadFromSession. Also, they should no longer call $user->loadFromSession() before checking whether a user is logged in or not.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links