What is the point of this line of code?

Question

I found this line of code in the Virtuemart plugin for Joomla on line 2136 in administrator/components/com_virtuemart/classes/ps_product.php

eval ("\$text_including_tax = \"$text_including_tax\";");

Answer 1

Scrap my previous answer.

The reason this eval() is here is shown in the php eval docs

This is what's happening:

$text_including_tax = '$tax <a href="...">...</a>';

...

$tax = 10;

...

eval ("\$text_including_tax = \"$text_including_tax\";");

At the end of this $text_including_tax is equal to:

"10 <a href="...">...</a>"

The single quotes prevents $tax being included in the original definition of the string. By using eval() it forces it to re-evaluate the string and include the value for $tax in the string.

I'm not a fan of this particular method, but it is correct. An alternative could be to use sprintf()

Answer 2

This code seems to be a bad way of forcing $text_including_tax to be a string.

The reason it is bad is because if $text_including_tax can contain data entered by a user it is possible for them to execute arbitrary code.

For example if $text_include_tax was set to equal:

"\"; readfile('/etc/passwd'); $_dummy = \"";

The eval would become:

eval("$text_include_tax = \"\"; readfile('/etc/passwd'); $_dummy =\"\";");

Giving the malicious user a dump of the passwd file.

A more correct method for doing this would be to cast the variable to string:

$text_include_tax = (string) $text_include_tax;

or even just:

$text_include_tax = "$text_include_tax";

If the data $text_include_tax is only an internal variable or contains already validated content there isn't a security risk. But it's still a bad way to convert a variable to a string because there are more obvious and safer ways to do it.

Answer 3

I'm guessing that it's a funky way of forcing $text_including_tax to be a string and not a number.

Answer 4

Perhaps it's an attempt to cast the variable as a string? Just a guess.

Answer 5

You will need the eval to get the tax rate into the output. Just moved this to a new server and for some reason this line caused a server error. As a quick fix, I changed it to:

//eval ("\$text_including_tax = \"$text_including_tax\";");
$text_including_tax = str_replace('$tax', $tax, $text_including_tax);

Answer 6

It is evaluating the string as PHP code.

But it seems to be making a variable equal itself? Weird.

Answer 7

As others have pointed out, it's code written by someone who doesn't know what on earth they're doing.

I also had a quick browse of the code to find a total lack of text escaping when putting HTML/URIs/etc. together. There are probably many injection holes to be found here in addition to the eval issues, if you can be bothered to audit it properly.

I would not want this code running on my server.

Answer 8

I've looked through that codebase before. It's some of the worst PHP I have seen.

I imagine you'd do that kind of thing to cover up mistakes you made somewhere else.

Answer 9

No, it's doing this:

Say $text_including_tax = "flat". This code evaluates the line:

$flat = "flat";

It isn't necessarily good, but I did use a technique like this once to suck all the MySQL variables in an array like this:

    while ($row = mysql_fetch_assoc($result)) {
        $var = $row["Variable_name"];
        $$var = $row["Value"];
    }