tags:

views:

1682

answers:

13

hi all,

i all the years i have been developing in php, i've always heard that using eval() is evil.

considering the following code, wouldn't it make sense, to use the second (and elegant) option? if no, why?

// $type is the result of an SQL statement
// e.g. SHOW COLUMNS FROM a_table LIKE 'a_column';
// hence you can be pretty sure about the consistency
// of your string
$type = "enum('a','b','c')";

// possibility one
$type_1 = preg_replace('#^enum\s*\(\s*\'|\'\s*\)\s*$#', '', $type);
$result = preg_split('#\'\s*,\s*\'#', $type_1);

// possibility two
eval('$result = '.preg_replace('#^enum#','array', $type).';');
+5  A: 

When you are using foreign data (such as user input) inside the eval.

In your example above, this isn't an issue.

GreenieMeanie
+19  A: 

eval is evil when there is only the slightest possibility that userinput is included in the evaluated string. When you do eval without content that came from a user, you should be safe.

But as a rule of thumb: Forget about it. When eval is the answer you're propably asking the wrong question! ;-)

Patrick Cornelissen
Sometimes eval IS the answer. We work on online-game application and it's very hard to avoid evals there because of very complex relations between the entities... but IMHO in 90% of cases eval is not the answer.
Jet
I'd be really curious to see a situation where ONLY eval is the answer.
Ionuț G. Stan
@Ionut G. Stan: When you are doing assertions?
SeanJA
@Ionut G. Stan Database-stored custom triggers for objects/entities?
Kuroki Kaze
Ionut, see my answer for example of justified use of eval().
Michał Rudnicki
@SeanJA, maybe.@Kuroki Kaze, I believe I need some code to better understand what you said.
Ionuț G. Stan
I don't really buy that any of those are justified uses of eval(). In each case, doing the same thing without using eval() is still at least possible. It's not like PHP is crippled without eval(). Granted, eval() is a shortcut in cases like these, but it still makes your code path that little bit harder to follow and problems that little bit harder to debug. That's my opinion.
thomasrutter
+4  A: 

I'd also pay some consideration to people maintaining your code.

eval() isn't the easiet to just look at and know what is supposed to happen, your example isn't so bad, but in other places it can be a right nightmare.

Phil Carter
+2  A: 

Personally, I think that code's still pretty evil because you're not commenting what it's doing. It's also not testing its inputs for validity, making it very fragile.

I also feel that, since 95% (or more) of uses of eval are actively dangerous, the small potential time saving that it might provide in other cases isn't worth indulging in the bad practice of using it. Plus, you'll later have to explain to your minions why your use of eval is good, and theirs bad.

And, of course, your PHP ends up looking like Perl ;)

There are two key problems with eval(), (as an "injection attack" scenario):

1) It may cause harm 2) It may simply crash

and one that's more-social-than-technical:

3) It'll tempt people to use it inappropriately as a shortcut elsewhere

In the first case, you run the risk (obviously, not when you're eval'ing a known string) of arbitrary code execution. Your inputs may not be as known or as fixed as you think, though.

More likely (in this case) you'll just crash, and your string will terminate with a gratuitously obscure error message. IMHO, all code should fail as neatly as possible, failing which it should throw an exception (as the most handleable form of error).

I'd suggest that, in this example, you're coding by coincidence rather than coding to behaviour. Yes, the SQL enum statement (and are you sure that field's enum? - did you call the right field of the right table of the right version of the database? Did it actually answer?) happens to look like array declaration syntax in PHP, but I'd suggest what you really want to do is not find the shortest path from input to output, but rather tackle the specified task:

  • Identify that you have an enum
  • Extract the inner list
  • Unpack the list values

Which is roughly what your option one does, but I'd wrap some if's and comments around it for clarity and safety (eg, if the first match doesn't match, throw exception or set null result).

There are still some possible issues with escaped commas or quotes, and you should probably unpack the data then de-quote it, but it does at least treat data as data, rather than as code.

With the preg_version your worst outcome is likely to be $result=null, with the eval version the worst is unknown, but at least a crash.

Parsingphase
+6  A: 

In this case, eval is probably safe enough, as long as it's never possible for arbitrary columns to be created in a table by a user.

It's not really any more elegant though. This is basically a text parsing problem, and abusing PHP's parser to handle is seems a bit hacky. If you want to abuse language features, why not abuse the JSON parser? At least with the JSON parser, there's no possibility at all of code injection.

$json = str_replace(array(
 'enum', '(', ')', "'"), array)
 '',     '[', ']', "'"), $type);
$result = json_decode($json);

A regular expression is probably the most obvious way. You can use a single regular expression to extract all the values from this string:

$extract_regex = '/
 (?<=,|enum\() # Match strings that follow either a comma, or the string "enum("...
 \'  # ...then the opening quote mark...
 (.*?)  # ...and capture anything...
 \'  # ...up to the closing quote mark...
 /x';
preg_match_all($extract_regex, $type, $matches);
$result = $matches[1];
BlackAura
even though this did not answer the question per se, this is a very good answer to the question: what would be the best way to parse an enum() … thank you ;)
Pierre Spring
+3  A: 

eval evaluates a string as code, the problem with that is if the string is in any way "tainted" it could expose huge security threats. Normally the problem is in a case where user input is evaluated in the string in many cases the user could input code (php or ssi for example) that is then run within eval, it would run with the same permissions as your php script and could be used to gain information/access to your server. It can be quite tricky to make sure user input is properly cleaned out before handing it to eval. There are other problems... some of which are debatable

Toby
+4  A: 

i'll blatantly steal the content here:

  1. Eval by its nature is always going to be a security concern.

  2. Besides security concerns eval also has the problem of being incredibly slow. In my testing on PHP 4.3.10 its 10 times slower then normal code and 28 times slower on PHP 5.1 beta1.

blog.joshuaeichorn.com: using-eval-in-php

Schnalle
+14  A: 

I would be cautious in calling eval() pure evil. Dynamic evaluation is a powerful tool and can sometimes be a life saver. With eval() one can work around shortcommings of PHP (see below).

The main problems with eval() are:

  • Potential unsafe input. Passing an untrusted parameter is a way to fail. It is often not a trivial task to make sure that a parameter (or part of it) is fully trusted.
  • Trickyness. Using eval() makes code clever, therefore more difficult to follow. To quote Brian Kernighan "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it"

The main problem with actual use of eval() is only one:

  • inexperienced developers who use it without enough consideration.

As mentioned before eval() can help you do things that are impossible in pure PHP. My favourite trick involving dynamic evaluation enables static calls on variable classes. Since $foo::bar() is illegal in PHP, below solution works around that limitation.

$className = 'Foo';
eval('$result = ' . $className . '::bar()');
echo $result;

As a rule of thumb I tend to follow this:

  1. Sometimes eval is the only/the right solution.
  2. For most cases one should try something else.
  3. If unsure, goto 2.
  4. Else, be very, very careful.
Michał Rudnicki
This could be refactored to avoid eval() too, especially if $className is whitelisted, which it must be in order to entertain the use of eval(). Good news is though, as of 5.3, $foo::bar() is valid.
rojoca
@rojoca: Can you give us example how to do it without eval() in PHP 5.2, please?
Michał Rudnicki
I'm not sure if it counts as eval or not, but $result = call_user_func(array('Foo', 'bar')); works like a charm.
Ionuț G. Stan
Interesting. Never thought about this one.
Michał Rudnicki
+1 for trickyness and the Kernighan quote.
Yar
Good point about trickyness - in your (simple) example, a variable pops out "from nowhere". If the code gets just a little bit more complex, good luck to the next person who looks at the code and tries to chase that variable down (been there, done that, all I got was a headache and this lousy t-shirt).
Piskvor
+2  A: 

PHP advises that you write your code in such a way that it can be executing via call_user_func instead of doing explicit evals.

Nolte Burke
+3  A: 

eval() is evil at all times.

"When is eval() not evil?" is the wrong question to be asking in my opinion, because it seems to misunderstand that the drawbacks to using eval() are always present, and are not dependent on context.

It is generally a bad idea because it decreases readability of code, the ability for you to predict the code path (and possible security implications of that) before runtime, and hence the ability to debug code. Furthermore, there is no situation for which it is absolutely necessary to use eval() - PHP is a completely functional programming language without it.

Whether or not you actually see these as evils or you can personally justify using eval() for some purposes despite this is up to you. To some, the evils are too great to ever justify it, and to others, eval() is a handy shortcut, voodoo or not.

However, if you see eval() as evil, it's evil at all times. It does not magically lose its evilness depending on context.

thomasrutter
+1  A: 

I used to use eval() a lot, but I found most of the cases you don't have to use eval to do tricks. Well, you have call_user_func() and call_user_func_array() in PHP. It's good enough to statically and dynamically call any method.

To perform a static call construct your callback as array('class_name', 'method_name'), or even as simple string like 'class_name::method_name'. To perform a dynamic call use array($object, 'method') style callback.

The only sensible use for eval() is to write a custom compiler. I made one, but eval is still evil, because it's so damn hard to debug. The worst thing is the fatal error in evaled code crashes the code which called it. I used Parsekit PECL extension to check the syntax at least, but still no joy - try to refer to unknown class and whole app crashes.

Harry
A: 

eval() is slow, but I wouldn't call it evil.

It's the bad use we make of it that can lead to code injection and be evil.

A simple example:

$_GET = 'echo 5 + 5 * 2;';
eval($_GET); // 15

A harmlful example:

$_GET = 'system("reboot");';
eval($_GET); // oops

I would advise you to not use eval() but if you do, make sure you validate / whitelist all input.

Alix Axel
+1  A: 

Another reason eval is evil is, that it could not cached by PHP bytecode caches like eAccelertor or ACP.

Hippo