What is so wrong with extract()?

Question

Hey everyone,

I was recently reading this thread, on some of the worst PHP practices. In the second answer there is a mini discussion on the use of extract(), and im just wondering what all the huff is about.

I personally use it to chop up a given array such as $_GET or $_POST where I then sanitize the variables later, as they have been conveniently named for me.

Is this bad practice? What is the risk here? What are your thoughts on the use of extract()?

Thank you,

Answer 1

The risk is the same as with register_globals. You enable the attacker to set variables in your script, simply by tampering with the request.

Answer 2

I guess the reason a lot of people don't recommend using it is that extracting $_GET and $_POST (even $_REQUEST) superglobals registers variables in the global namespace with the same name as each key within those arrays, which is basically emulating REGISTER_GLOBALS = 1.

Answer 3

I find that it is only bad practice in that it can lead to a number of variables which future maintainers (or yourself in a few weeks) have no idea where they're coming from. Consider this scenario:

extract($someArray); // could be $_POST or anything

/* snip a dozen or more lines */

echo $someVariable;

Where did $someVariable come from? How can anyone tell?

I don't see the problem in accessing the variables from within the array they started in, so you'd really need to present a good case for using extract() for me to think it's worth it. If you're really concerned about typing out some extra characters then just do this:

$a = $someLongNameOfTheVariableArrayIDidntWantToType;

$a['myVariable'];

I think the comments here on the security aspects of it are overblown somewhat. The function can take a second parameter that actually gives you fairly good control over the newly created variables, including not overwriting any existing variables (EXTR_SKIP), ONLY overwriting existing variables (so you can create a whitelist) (EXTR_IF_EXISTS), or adding prefixes to the variables (EXTR_PREFIX_ALL).

Answer 4

the risk is: don't trust data from users, and extracting into the current symbol table means, your variables could be overwritten by something the user provides.

<?php
    $systemCall = 'ls -lh';
    $i = 0;

    extract($_GET);

    system($systemCall);

    do {
        print_r($data[$i];
        $i++;
    } while ($i != 3);

?>

(a nonsensical example)

but now a malicious user who guesses or knows the code calls:

yourscript.php?i=10&systemCall=rm%20-rf

instead of

yourscript.php?data[]=a&data[]=b&data[]=c

now, $systemCall and $i are overwritten, resulting in your script deleting your data first and hanging then.

Answer 5

There is nothing wrong with it. Otherwise it wouldnt be implemented. Many (MVC) frameworks use it when you pass (assign) variables to Views. You just need to use it carefully. Sanitize those arrays before passing it to extract() and make sure it does not override your variables. Dont forget that this function also accepts a few more arguments! Using the second and third arguments you can control the behavior if collision occurs. You can override, skip or add prefix. http://www.php.net/extract

Answer 6

Come on now. People blame the tool instead of the user.

That's like talking against unlink() because you can delete files with it. extract() is a function like any other, use it wisely and responsibly. But don't claim it's bad per se, that's just ignorant.

Answer 7

I'll let the PHP manual do the talking for me.

Background: extract($_REQUEST) is the same as setting register_globals = On in php.ini

Answer 8

If you extract in a function, the variables will only be available in that scope. This is often used in views. Simple example:

//View.php
class View {
    function render($filename = null) {
     if ($filename !== null) {
      $this->filename = $filename;
     }
     unset($filename);
     extract($this->variables);
     ob_start();
     $this->returned = include($this->dir . $this->filename);
     return ob_get_clean();
    }
}

//test.php
$view = new View;
$view->filename = 'test.phtml';
$view->dir = './';
$view->variables = array('test' => 'tset');
echo $view->render('test.phtml');
var_dump($view->returned);

//test.phtml
<p><?php echo $test; ?></p>

With some alternative directories, checks to see if the file exists and defined variables and methods - you've pretty much replicated Zend_View.

You can also add *$this->outVariables = get_defined_vars();* after the include to run code with specific variabels and get the result of these for use with old php code.

Answer 9

Never extract($_GET) in a global scope. Other than that, it has its uses, like calling a function that could (potentially) have lots of optional arguments.

This should look vaguely familiar to WordPress developers:

function widget (Array $args = NULL)
{
    extract($args);

    if($before_widget) echo $before_widget;

    // do the widget stuff

    if($after_widget) echo $after_widget;
}

widget(array(
    'before_widget' => '<div class="widget">',
    'after_widget' => '</div>'
));

Answer 10

If not used carefully it can confuse the heck out of others you work with consider:

<?php

    $array = array('huh' => 'var_dump', 'whatThe' => 'It\'s tricky!', 'iDontGetIt' => 'This Extract Function');
    extract($array);
    $huh($whatThe, $iDontGetIt);


?>

Yields:

string(12) "It's tricky!"
string(21) "This Extract Function"

Would be useful to use in an obfuscation. But I can't get over the "Where did that var come from?" problem that I run into.

Answer 11

People get all up-in-arms about extract because it has the potential to be misused. Doing something like extract($_POST) is not a good idea in any case, even if you know what you are doing. However, it does have it's uses when you are doing things like exposing variables to a view template or something similar. Basically, only use it when you are very certain that you have a good reason for doing so, and understand how to use the extract type parameter if you get the idea of passing something crazy like $_POST to it.

Answer 12

As someone noted in a different thread, here is a safer way to use extract, by only allowing it to extract variables you specify, instead of everything the array contains.

This serves a dual purpose of documenting what variables are coming out of it so tracking back a variable wont be so difficult.