Simple Machines Community Forum

SMF Development => Bug Reports => Fixed or Bogus Bugs => Topic started by: Chen Zhen on March 05, 2013, 01:06:52 AM

Title: fatal error in ManageMembers.php
Post by: Chen Zhen on March 05, 2013, 01:06:52 AM

I'm working on something & found an error in the code.

Fatal error: Unsupported operand types

File:
/Sources/ManageMembers.php

find:

if ($context['sub_action'] == 'query' && !empty($_REQUEST['params']) && empty($_POST))
$_POST += @unserialize(base64_decode($_REQUEST['params']));


if $_POST is empty then just use equals since it is not adding anything to NULL
...

if ($context['sub_action'] == 'query' && !empty($_REQUEST['params']) && empty($_POST))
$_POST = @unserialize(base64_decode($_REQUEST['params']));


test it as such:
../index.php?action=admin;area=viewmembers;sa=query;params=whatever
Title: Re: fatal error in ManageMembers.php
Post by: Shambles on March 05, 2013, 05:13:06 AM
Is it complaining that

$_POST += @unserialize(base64_decode($_REQUEST['params']));

should be

$_POST .= @unserialize(base64_decode($_REQUEST['params']));


?
Title: Re: fatal error in ManageMembers.php
Post by: emanuele on March 05, 2013, 07:16:54 AM
Nope, it should be plain and simple equal as suggested by the OP. ;)
Title: Re: fatal error in ManageMembers.php
Post by: Shambles on March 05, 2013, 07:23:32 AM
Syntactically tho, the .= is correct I gather?
Title: Re: fatal error in ManageMembers.php
Post by: emanuele on March 05, 2013, 07:34:49 AM
No, it is not.

.= concatenates strings.
+= "merge" arrays.
= assigns and remove anything that was there previously.

empty($something) could mean that $something is one of: not set at all, null, false, '' (i.e. empty string), 0 (zero), array() (an empty array).
As far as I know, $_POST should always be initialized at an array (so I'm not sure what the OP is doing, but he may have altered the value of $_POST and that's bad, but cannot be sure).
Though I doubt using "=" is a good thing because if unserialize fails $_POST will become something different from an array (false in particular), and that is bad, I think.
Title: Re: fatal error in ManageMembers.php
Post by: Chen Zhen on March 05, 2013, 05:04:07 PM
Quote from: emanuele on March 05, 2013, 07:34:49 AM
No, it is not.

.= concatenates strings.
+= "merge" arrays.
= assigns and remove anything that was there previously.

empty($something) could mean that $something is one of: not set at all, null, false, '' (i.e. empty string), 0 (zero), array() (an empty array).
As far as I know, $_POST should always be initialized at an array (so I'm not sure what the OP is doing, but he may have altered the value of $_POST and that's bad, but cannot be sure).
Though I doubt using "=" is a good thing because if unserialize fails $_POST will become something different from an array (false in particular), and that is bad, I think.

emanuele,

The condition asks if empty (NULL) and then attempts to add to its array with += ... If it is empty += is not what is used.
In this case $_POST has a possible array of data from a form or from the url, correct?
The value of $_POST in this case is being altered via the url. What I am getting at is fixing it so no error occurs in any event.
I am assuming of course that it is perhaps the developers intent that php fatal errors do not occur with what is typed in the url (serious statement - not sarcasm. please do not misunderstand.).

Perhaps this is more prudent and does not create an error:

if ($context['sub_action'] == 'query' && !empty($_REQUEST['params']) && count($_POST)>0)
$_POST += @unserialize(base64_decode($_REQUEST['params']));
elseif ($context['sub_action'] == 'query' && !empty($_REQUEST['params']))
$_POST = @unserialize(base64_decode($_REQUEST['params']));


or..


if ($context['sub_action'] == 'query' && !empty($_REQUEST['params']) && !empty($_POST))
$_POST += @unserialize(base64_decode($_REQUEST['params']));
elseif ($context['sub_action'] == 'query' && !empty($_REQUEST['params']))
$_POST = @unserialize(base64_decode($_REQUEST['params']));


Title: Re: fatal error in ManageMembers.php
Post by: Arantor on March 05, 2013, 05:10:21 PM
QuoteThe condition asks if empty

empty() matches more than null, it matches '', 0 and an array with no elements.

In fact, cleanRequest() in QueryString.php should be preventing the case of $_POST not being an array which means something breaks it between initial definition and that point in time.
Title: Re: fatal error in ManageMembers.php
Post by: emanuele on March 05, 2013, 05:39:39 PM
Ahhh...now I got it.
The issue is exactly when unserialize fails, it returns false and the error is raised.

ETA: in 2.1 is already fixed by not assigning anything to $_POST:
https://github.com/SimpleMachines/SMF2.1/blob/master/Sources/ManageMembers.php#L283
Title: Re: fatal error in ManageMembers.php
Post by: Arantor on March 05, 2013, 05:42:54 PM
Better question, why does unserialize fail?
Title: Re: fatal error in ManageMembers.php
Post by: emanuele on March 05, 2013, 05:44:49 PM
Because when he said "whatever" in the example URL, he was literally implying "whatever"...

ETA: don't ask me why... :P
Title: Re: fatal error in ManageMembers.php
Post by: Chen Zhen on March 05, 2013, 06:07:26 PM
Quote from: Arantor on March 05, 2013, 05:42:54 PM
Better question, why does unserialize fail?

Hmm, yes..


if ($context['sub_action'] == 'query' && !empty($_REQUEST['params']) && empty($_POST))
$_POST += (array)@unserialize(base64_decode($_REQUEST['params']));


I gather the parameter being added is not an array in this example, (array) causes it to be an array no matter what.
Title: Re: fatal error in ManageMembers.php
Post by: emanuele on March 06, 2013, 02:27:12 AM
Quote from: emanuele on March 05, 2013, 05:39:39 PM
ETA: in 2.1 is already fixed by not assigning anything to $_POST:
https://github.com/SimpleMachines/SMF2.1/blob/master/Sources/ManageMembers.php#L283

;)