miniBB ® 

miniBB

®
Support Forums		   
 | Start | Register | Search | Statistics | File Bank | Manual |
Suggestions miniBB Support Forums / Suggestions /  
 

Unsetting conditions for the $unset array in index.php file

 
Author tom322
Active Member		 #1 | Posted: 6 Jan 2019 21:14 
I have quick question about code - in the index.php file there is like this on top:

$unset=array('logged_admin', 'isMod', 'user_id', 'langu', 'includeHeader', 'includeFooter', 'emptySubscribe', 'allForumsReg', 'registerInactiveUsers', 'mod_rewrite', 'enableViews', 'userDeleteMsgs', 'userInfoInPosts', 'inss', 'insres', 'preModerationType', 'textLd', 'adminAcceptsSignup', 'customProfileList', 'correct', 'customTopicSort', 'manualIndex', 'startIndex', 'mTop', 'mdrw', 'metaLocation', 'post', 'reply_to_email', 'csrfchk', 'emailCharset', 'adminUser', 'cook', 'forumClone', 'xtr', 'addMainTitle', 'url_follow', 'site_url', 'allowedRefs', 'uname_minlength', 'uname_maxlength', 'enchecked', 'fIconWidth', 'fIconHeight', 'smartLinking', 'pathToTpl', 'archives', 'forumsTt', 'forums_url', 'mysql_set_names', 'eeol', 'startNewTopicLink', 'lastPostIcon', 'disableNavDisplay', 'custom_mobile_actions', 'disableSuperSticky','superStickyModule', 'check_pmquota', 'failedSending', 'rheader', 'allowHyperlinksProfile', 'l_mobilePreviousPage', 'l_mobileNextPage', 'statsDefField', 'emailname', 'minibb_copyright_txt', 'disableDates', 'brtag', 'lastPosterIcon', 'dstr');

for($i=0;$i<sizeof($unset);$i++) if(isset(${$unset[$i]})) { ${$unset[$i]}=''; unset(${$unset[$i]}); }
Is this:
${$unset[$i]}='';
needed? I'm not sure if it's needed to set this when we know this value is already set and script unsets it anyway.. maybe it was from some old code.. It would save some ms if it's not needed :

OR - maybe even better and faster would be to just unset the whole array like:

unset($unset);
My goal is to make it the fastest (not sure if foreach or just unset whole array would be faster).

Author tom322
Active Member		 #2 | Posted: 6 Jan 2019 21:27 
PS - from http://php.net/manual/en/function.unset.php#111576 - it seems that when using unset it's not even needed to check if variable is set, so my two proposed solutions (if they are valid) would be:

unset($unset);
OR

for($i=0;$i<sizeof($unset);$i++) unset(${$unset[$i]});
(depending on what is better/faster).

Author tom322
Active Member		 #3 | Posted: 6 Jan 2019 23:33 
Ok I did some benchmark test and this is clearly the fastest (more than 2.5 times faster than the current code):

unset($unset);
So, if it is correct, this:

for($i=0;$i<sizeof($unset);$i++) if(isset(${$unset[$i]})) { ${$unset[$i]}=''; unset(${$unset[$i]}); }
could be changed to:

unset($unset);

Author tom322
Active Member		 #4 | Posted: 7 Jan 2019 05:57 
Now I think not all assumptions above are correct (it's about resetting variables starting with $, not just variables in the array), so my final proposal would be to leave the original one but without unnecessary checking if the variables are set and without assigning an empty value, like this:

for($i=0;$i<sizeof($unset);$i++) unset(${$unset[$i]});
That should be the fastest and work without conflicts. Is that right? : )

Author Paul
Lead Developer 		 #5 | Posted: 9 Jan 2019 23:55 
tom322:
maybe it was from some old code
Obviously, yes. It comes from some ancient code, and you/PHP docs are right: just putting

for($i=0;$i<sizeof($unset);$i++) unset(${$unset[$i]});
would be enough. Thanks for keeping an eye on it, I'll fix it in the next sub-release.

! This is an improper approach:

unset($unset);
In this code, we need to unset all possible critical variables which may be sent from outside, if PHP's option register_globals is set to ON. I know it's an ancient option and nowadays, on most secured hosting servers, it's set to OFF (which is correct and which is how it should be). These variables' names are defined in the $unset array and then are being unset while going through array's elements, which in the cycle are transformed to variables. If we just unset the array itself, it's about another thing and it will not work for the purpose.

It's good you did some benchmark test but I suppose, such small array couldn't really affect lots of things :) it's just a simple memory operation. Still it truly deserves the mini approach we're always trying to achieve in this project. Thanks :)

Author tom322
Active Member		 #6 | Posted: 10 Jan 2019 00:01 
Thank you (was a little worried about late response ;) - but yes, now it all makes sense and I'll implement this change; yes, mini is always better, especially for such obvious things that may give like 10ms speed advantage (considering that index.php is loaded with all pages too).

Author tom322
Active Member		 #7 | Posted: 10 Jan 2019 00:35 
Paul:
In this code, we need to unset all possible critical variables which may be sent from outside, if PHP's option register_globals is set to ON
Hm, does it mean that if register_globals = OFF Then this loop is not needed at all (I see register globals was removed in PHP 5.4 - php.net/manual/en/security.globals.php) ? Ie. this code below can be removed or commented? If so, that would speed things up even more:

$unset=array('logged_admin', 'isMod', 'user_id', 'langu', 'includeHeader', 'includeFooter', 'emptySubscribe', 'allForumsReg', 'registerInactiveUsers', 'mod_rewrite', 'enableViews', 'userDeleteMsgs', 'userInfoInPosts', 'inss', 'insres', 'preModerationType', 'textLd', 'adminAcceptsSignup', 'customProfileList', 'correct', 'customTopicSort', 'manualIndex', 'startIndex', 'mTop', 'mdrw', 'metaLocation', 'post', 'reply_to_email', 'csrfchk', 'emailCharset', 'adminUser', 'cook', 'forumClone', 'xtr', 'addMainTitle', 'url_follow', 'site_url', 'allowedRefs', 'uname_minlength', 'uname_maxlength', 'enchecked', 'fIconWidth', 'fIconHeight', 'smartLinking', 'pathToTpl', 'archives', 'forumsTt', 'forums_url', 'mysql_set_names', 'eeol', 'startNewTopicLink', 'lastPostIcon', 'disableNavDisplay', 'custom_mobile_actions', 'disableSuperSticky','superStickyModule', 'check_pmquota', 'failedSending', 'rheader', 'allowHyperlinksProfile', 'l_mobilePreviousPage', 'l_mobileNextPage', 'statsDefField', 'emailname', 'minibb_copyright_txt', 'disableDates', 'brtag', 'lastPosterIcon', 'dstr', 'is_mobile_test', 'is_mobile_browser', 'is_mobile', 'l_mobilePages', 'startReplyLink', 'chunkStr', 'splitExpression', 'l_pageN', 'bbimgmrg', 'bzoutput', 'firstPageTopicForm', 'viewUnameLngLim', 'is_mobile_samsung');

for($i=0;$i<sizeof($unset);$i++) if(isset(${$unset[$i]})) { ${$unset[$i]}=''; unset(${$unset[$i]}); }

Author Paul
Lead Developer 		 #8 | Posted: 10 Jan 2019 00:45 
tom322:
Hm, does it mean that if register_globals = OFF Then this loop is not needed at all
I suppose so - unless I'm not sure if "existing" variables could be passed to PHP some other way. Right now there's no way for this (if register_globals is turned OFF or abandoned), but I'm not sure about possible "expoits", "bugs", whatever follows the PHP history.

Anyway - this code is a piece of cake... Any simple request to mySQL database takes much more resources than this, so you could remove it on your own risk, as usually in the open source :)

Author tom322
Active Member		 #9 | Posted: 10 Jan 2019 22:03 
I commented this part for a day so far and there are no warnings or notices so it may run well I hope :

Suggestions miniBB Support Forums / Suggestions /
 Unsetting conditions for the $unset array in index.php file
 Share Topic's Link

Your Reply Click this icon to move up to the quoted message
 ∅ ?
Post as a Guest, leaving the Password field blank. You could also enter a Guest name, if it's not taken by a member yet. Sign-in and post at once, or just sign-in, bypassing the message's text.

Before posting, make sure your message is compliant with forum rules; otherwise it could be locked or removed with no explanation.

 
 
miniBB Support Forums Powered by Forum Software miniBB ® Home  Features  Requirements  Demo  Download  Showcase  Gallery of Arts
Compiler  Premium Extensions  Premium Support  License  Contact Us
Try the Captcha add-on: protect your miniBB-forums from the automated spam and flood.


  ⇑