Posted by gjf on April 29, 2005 at 6:53am
| Project: | Drupal core |
| Component: | user system |
| Category: | bug report |
| Priority: | minor |
| Assigned: | deekayen |
| Status: | closed (fixed) |
Issue Summary
user_access() function returns a string not a boolean.
This caused memory consumption problems for us due to a bug elsewhere in our code.
i.e.
return strstr($perm[$account->uid], "$string, ");should probably be
if ( strstr($perm[$account->uid], "$string, "))
{
return true;
} else
{
return false;
}
Comments
#1
Attached patch has a spelling correction and makes user_access() return only booleans. Diff'ed against HEAD.
#2
It bothered me to have a string search instead of an array search for permissions. This patch does what the previous patch does, but it uses in_array() on the static $perm instead of strstr() on a string.
#3
#4
Can you please provide a benchmark test of the strstr approach vs the in_array one? I am concerned that it might be slower. user_access gets called quite often.
#5
That's a fair request. The output of the following code on my WinXP, PHP5.1-dev, 2.133 Ghz Athlon is:
0.29749894142151
0.29712684249878
1.0804200172424
0.56814384460449
0.10924220085144
0.12113380432129
0.13284420967102
<?php
function utime($a = '')
{
static $utime;
$time = explode(' ', microtime());
$usec = (double)$time[0];
$sec = (double)$time[1];
switch($a) {
case 'bm':
$to_return = $usec + $sec - $utime;
break;
default:
$utime = $usec + $sec;
return $utime;
break;
}
$utime = $usec + $sec;
return $to_return;
}
$account->uid = 1;
utime();
for($i=0; $i<500000; $i+=1) {
if ($account->uid == 1) {
// blah
}
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i+=1) {
if ((int)$account->uid === 1) {
// blah
}
}
echo utime('bm') .'<br />';
for($i=0; $i<50; $i+=1) {
$result[$i] = '';
for($j=0; $j>10; $j+=1) {
$result[$i] .= chr(rand(65-126));
}
}
$perm[$account->uid] = '';
utime();
for($i=0; $i<2000; $i+=1) {
foreach($result as $row) {
$perm[$account->uid] .= "$row, ";
}
isset($perm[$account->uid]) && strstr($perm[$account->uid], "{$result['45']}, ");
}
echo utime('bm') .'<br />';
$perm[$account->uid] = array();
utime();
for($i=0; $i<2000; $i+=1) {
foreach($result as $row) {
$perm[$account->uid][] = $row;
}
isset($perm[$account->uid]) && in_array($result['45'], $perm[$account->uid]);
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i+=1) {
// blah
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i++) {
// blah
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i = $i+1) {
// blah
}
echo utime('bm') .'<br />';
?>
#6
Actually, there's a bug in the benchmark code that causes $result to not populate. Switch the following:
for($j=0; $j>10; $j+=1) {to
for($j=0; $j<10; $j+=1) {Then the string search is more like 8.1727991104126 seconds verses like 0.46953892707825 for the array search, an even bigger difference.
#7
Committed to HEAD.
#8
Please revert this patch, it isn't working at all. Drupal HEad is non-workign for all users which aren't No. 1.
#9
yeah - this breaks HEAD completely. here is a possible fix, if we still want to use in_array() for checking. this is in user_access() where we build the permission array:
<?php
while ($row = db_fetch_object($result)) {
foreach (explode(',', $row->perm) as $p) {
$perm[$account->uid][] = trim($p);
}
}
?>
#10
I haven't found a case where trim() would be necessary if you explode on ', ' (with the space). I made a patch so things are hopefully less broken. I'm working on the benchmark people keep asking me for in #drupal.
#11
in_array() was faster, but explode() is slow, so this reverts back to strstr. I hope I can blame my newbieness to Drupal internals on this, but I should have looked to see how permissions were stored before just assuming each loop through db_fetch_object was a separate permission instead of a string of permissions.
#12
and the fact that you obviously did not test your patch with any user beside uid=1. considering that the code branch you changed doesn't even apply to uid=1, i'd say that this was pretty reckless.
other than that, welcome to the project. we look forward to your future patches ...
#13
the original issue was fixed in core.
#14
#15
#16
#17
#18