HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Using `exec()` in an autoloader

Submitted by: @import:stackexchange-codereview··
0
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?

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:

  • exec: sometimes, functions like passthru, exec an 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: find is not available, what then? Your autoloader won't work. Another scenario is one where find is 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, under Db/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 UniversalClassLoader will 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.