patternphpMinor
Using `exec()` in an autoloader
Viewed 0 times
execautoloaderusing
Problem
After reviewing autoloader functions on the site, I didn't see anything like this, and I was wonder what may be wrong with this?
The shell command returns
And of course, it could be used with
It could then be shortened into:
And that looks quite elegant, to me.
Or if you want to be safe:
Any thoughts ?
function findNloader($classFile){
$file = exec("find ./ -name ".$classFile.".php");
if(file_exists($file)){
return require $file;
} else {
return false;
}
}The shell command returns
false if somebody were to ever manage to inject something like: ../../../../sudoers or the like... And of course, it could be used with
spl_autoload_register() -- and if the file path were configured properly, it could find anything without chaining down autoload functions. I understand that if it were pointed at a very large project, it might be resource intensive -- but with a direct enough focus, is there anything else wrong with this?It could then be shortened into:
public function findnload($class) {
return (file_exists($st = exec("find ./ -name ".$class.".php")) ?
require $st : false);
}And that looks quite elegant, to me.
Or if you want to be safe:
public function findnload($class) {
return (file_exists($st = exec("find ./ -name ".$class.".php")) &&
(length($class) < $thirtyTwoOrSo) || ($someOtherSafetyCriteria)
?
require $st : false);
}Any thoughts ?
Solution
There are good reasons why you don't find (many, if any) examples of autoloaders or other functions that are built around the 2 things you are relying on:
So why is
Imagine using a modern dependency, like doctrine. Somewhere in your code, the
What is the value of
Now I'm not going into details, but I'm assuming it's pretty obvious that those backslashes are a problem. And a big one at that. Because you're concatenating them into a new string, the backslashes aren't properly escaped.But the biggest problem, by a country mile, is the simple fact that, even if properly escaped, the
Other problems:
Not only are you not checking the exit status of
Rule one when writing vital components of any system, be it a web-app or a low-level piece of embedded software: Embrace Murphy's laws: Whatever can go wrong, will go wrong.
Add to that the mantra of webdevelopment: Never trust the network, and I think it safe to say that, in an attempt to write a short/small autoloader, you've undermined reliability, scalability and general security. Not a fair trade-off if you ask me.
Things to consider:
If you have a bigger project, read through the composer docs. Composer creates autoloaders, that use classmaps, and other simple, but clever tricks to ensure the right class is loaded at the right time, with minimal overhead.
One of the things that sets a programmer apart is being lazy, in a clever way. Not wasing time, effort and brain-power on something that has been written before is one of those examples of clever laziness.
exec: sometimes, functions likepassthru,execan the like are your only option. But not when it comes to auto-loading classes. You're not using the right tool for the job, and are in fact using what can only be described as a dangerous function. More on this later.
find: 2 problems that might occur here:findis not available, what then? Your autoloader won't work. Another scenario is one wherefindis present, but the user running PHP, does not have the rights required to use it
So why is
exec dangerous?Imagine using a modern dependency, like doctrine. Somewhere in your code, the
ClassLoader class is used. Your autoloader kicks in:function findNloader($classFile)
{
$file = exec("find ./ -name ".$classFile.".php");
if(file_exists($file))
{
}
}What is the value of
$classFile going to be? Well, doctrine's ClassLoader class is namespaced, its full name, and with it the value of $classFile will be Doctrine\Common\ClassLoader. Pass that through to your function, and you'll get:$file = exec("find ./ -name Doctrine\Common\ClassLoader.php");Now I'm not going into details, but I'm assuming it's pretty obvious that those backslashes are a problem. And a big one at that. Because you're concatenating them into a new string, the backslashes aren't properly escaped.But the biggest problem, by a country mile, is the simple fact that, even if properly escaped, the
\ character assumes a different meaning depending on the OS.Other problems:
Not only are you not checking the exit status of
find, you're not capturing the full output of the command, and worst of all: You're not handling possible errors.Rule one when writing vital components of any system, be it a web-app or a low-level piece of embedded software: Embrace Murphy's laws: Whatever can go wrong, will go wrong.
Add to that the mantra of webdevelopment: Never trust the network, and I think it safe to say that, in an attempt to write a short/small autoloader, you've undermined reliability, scalability and general security. Not a fair trade-off if you ask me.
Things to consider:
- Namespaces: As I've already explained above, your autoloader does not handle namespaces.
- Pseudo-namespaces: In the pre-namespace era, most sizable projects were pseudo-namespaced. Think Zend Framework 1.x and the like. Classes had names like
Zend_Db_Table_Abstract, and these classes were located in the Zend root directory, underDb/Table. The underscores formed the path. Your autoloader doesn't take those (very common) cases into account.
- Don't reinvent the wheel, be lazy. Many autoloaders have been written before, and many will be written in the future. But generally, things like Symfony2's excellent, and easy to use
UniversalClassLoaderwill work fine for small, to mid-sized projects.
If you have a bigger project, read through the composer docs. Composer creates autoloaders, that use classmaps, and other simple, but clever tricks to ensure the right class is loaded at the right time, with minimal overhead.
One of the things that sets a programmer apart is being lazy, in a clever way. Not wasing time, effort and brain-power on something that has been written before is one of those examples of clever laziness.
Code Snippets
function findNloader($classFile)
{
$file = exec("find ./ -name ".$classFile.".php");
if(file_exists($file))
{
}
}$file = exec("find ./ -name Doctrine\Common\ClassLoader.php");Context
StackExchange Code Review Q#56903, answer score: 4
Revisions (0)
No revisions yet.