[eluser]Alex.[/eluser]
Lines 672 - 716 of system/libraries/Upload.php
Code: /**
* Clean the file name for security
*
* @access public
* @param string
* @return string
*/
function clean_file_name($filename)
{
$bad = array(
"<!--",
"-->",
"'",
"<",
">",
'"',
'&',
'$',
'=',
';',
'?',
'/',
" ",
""",
"<", // <
"%3c", // <
">", // >
"", // >
"(", // (
")", // )
"%28", // (
"&", // &
"$", // $
"?", // ?
";", // ;
"=" // =
);
foreach ($bad as $val)
{
$filename = str_replace($val, '', $filename);
}
return stripslashes($filename);
}
Would it not be better to just pass the $bad variable as the first parameter like so?:
Code: str_replace($bad, '', $filename);
Thus avoiding the need for the loop.
[eluser]Randy Casburn[/eluser]
Nice catch actually. This would not seem to break backward compatibility with 4.3.2 forward. There are no outstanding related str_replace() bugs in the bug DB. But I'm a nobody with no say in the matter. Even faster and faster we go! 
[eluser]Derek Jones[/eluser]
I'd have to go look at the source code to check, but I'm pretty sure str_replace() with an array does the same thing internally as that current code is doing in PHP, i.e. looping through an array and calling the actual replacement method over and over again. If memory serves, it even works on a copy of the array just like foreach() does. Not arguing against the code change, I agree it's simpler to just shove the array, but I don't think there's any performance or memory to be gained.
[eluser]Randy Casburn[/eluser]
Hi Derek, I'm not that close to the PHP source but I don't understand. Are you implying that the direct C implementation of array manipulation inside the PHP engine would be on par memory & cycle wise as the foreach() implementation running through the interpreter?
Interesting.
Randy
[eluser]Derek Jones[/eluser]
Been quite some time since I examined it, but yes, that's the implication if I'm recalling correctly how it's handled.
[eluser]Randy Casburn[/eluser]
Don't mean to dispute you, I'm just in constant learning mode. Hope you understand.
I've attached the header zend_iterators.h you'll see in it that it states clearly all the foreach() implementations use the iterators declared there. Hence the interpreter uses these iterators.
I've also included an excerpt from string.c which is the c function declaration for str_replace. There you will see that the function has it's own internal array iterator and does not call the common iterator mentioned above.
I think it unlikely doing things both ways would have the same results.
It's more likely this is a quality input and should be patched in the next revision. But as my prior post said. I'm a nobody with no say, nor credibility here.
Hope this is helpful. I truly did not mean this as a challenge - this was a learning event for me personally.
Randy
[edit -- ok - what formats 'can' I upload? ]
[eluser]Randy Casburn[/eluser]
Derek, Files sent via PM.
Randy
[eluser]Derek Jones[/eluser]
Challenge forces growth, Randy, so no worries. I'm not seeing any attachments or samples in your post, but a quick download of the source confirms your assessment. For each element, you save yourself a conditional in the source to only call str_replace() once, in addition to the "overhead" of a function call. We're talking minutia here, even on large arrays, but it's still better code and a dead simple change to make.
[eluser]Derek Jones[/eluser]
Posted before refreshing to see your edit. Zip files are generally the best attachment type to use here in the forums. .h and .c don't have a correlative MIME type, so they would be disallowed to avoid uploading content that might execute in the user's browser.
[eluser]Randy Casburn[/eluser]
My assessment is different. This is more than just "the overhead of a function call". We're on different plains here simply because I'm considering the boundary (inside the PHP core vs. interpreted by PHP) as the defining issue. Specifically, as the files I sent you make clear:
1) If you put the array inside the call to str_replace() the iterations over the array are handled in C constructs inside the core PHP engine.
2) If you put the str_replace() function inside a foreach loop the iterations of the foreach() loop are handled by array iterators in the interpreter engine outside the core PHP engine requiring communication to and from the core to achieve the iterations.
The difference, while admittedly minuscule, on a human or computational scale is important because these are the reasons CI is the fastest thing going right now
Randy
|
