Comment On Eval This!

While reviewing some old JavaScript code in his company's core application, Dan Howard came across this pain. [expand full text]
« PrevPage 1 | Page 2Next »

Re: Eval This!

2007-08-08 09:32 • by FredSaw
try {eval(fist)}catch(e){delete}

Re: Eval This!

2007-08-08 09:37 • by dkf (unregistered)
Ugh, that style of coding looks somewhat catching...

Re: Eval This!

2007-08-08 09:41 • by b0bg0ats3 (unregistered)
FIST@!!!@!

Re: Eval This!

2007-08-08 09:45 • by FredSaw
148666 in reply to 148665
b0bg0ats3:
FIST@!!!@!
Actually, that would be "TURD@!!!@!", wouldn't it?

Re: Eval This!

2007-08-08 09:57 • by s0be
Normally I can find a way to see what the coder was thinking, but I just can't find ANY justification for this method of doing this.

Re: Eval This!

2007-08-08 09:58 • by n9ds
Well, that would make debugging easier, right? All you need to do is add alert(jStr); before each line you want to debug, and you know exactly where you are in the code! Brilliant! Unless you need to eval("alert(" + jStr + ")");

Re: Eval This!

2007-08-08 09:59 • by s (unregistered)
for(i=0;i<code.length;i++)
{
eval(code[i]);
}

I'm afraid it wouldn't work as intended...

Re: Eval This!

2007-08-08 10:01 • by Stiggy (unregistered)
I'd Evaluate that as being of poor quality.

Did I say that out loud?

2007-08-08 10:05 • by Grant (unregistered)
I think I almost actually said WTF out loud reading that! It makes it slower, harder to read, and more fragile all at the same time, and all with no benefit what so ever.

That is worse than ANYTHING I have ever written, and I have been coding since 1977.

Re: Eval This!

2007-08-08 10:29 • by bstorer
Hmmm... is that a code injection vulnerability I smell?

Re: Eval This!

2007-08-08 10:35 • by SomeCoder (unregistered)
About a year ago, I inherited a Perl script that was written a lot like this. Basically it had this structure:

eval
{
# Tons of Perl code
}

if ($@) {
# Handle error.
}

He was using it as a try/catch block for Perl. The thing is, the "Tons of Perl code" had plenty of error checking and handling that he could have done for it without the eval and all this monstrosity did was make it nearly impossible to debug.

Won't someone please think of the maintenance programmers?

Re: Eval This!

2007-08-08 10:46 • by durnurd
148678 in reply to 148676
bstorer:
Hmmm... is that a code injection vulnerability I smell?


No... not really in that case.

Maybe the programmer assumed that javascript had some sort of difference with late/early binding, and window.opener, since it may not exist, has to be evaluated as a string, so that it can be bound. Of course, this is silly, but it's the best I can come up with.

On another note (OT), while I'm here, what's the point of assert(0 == 1) in a language that supports throw? I just saw it in some of the code I'm debugging.

Addendum (2007-08-08 10:52):
Oh, wait, I skimmed over the code, not noticing the CompanyName. So there's a possibility for code injection, but it's Javascript. You'd only be hurting yourself.

Re: Eval This!

2007-08-08 10:48 • by qsdfqsdfsfsdfsqdfffffffffffffff (unregistered)
148679 in reply to 148674
Grant:
I think I almost actually said WTF out loud reading that! It makes it slower, harder to read, and more fragile all at the same time, and all with no benefit what so ever.

That is worse than ANYTHING I have ever written, and I have been coding since 1977.


It does have a benefit!

What do you think would happen without the eval when PolicyNumber does not exist?!

Didn't anyone tell you that this is the only way to be sure execution proceeds normally when the element doesn't exists?!

Re: Eval This!

2007-08-08 10:52 • by Pilum (unregistered)
Another big WTF is using document.all to access an element.

Re: Eval This!

2007-08-08 10:53 • by dunnomattic
...to quote Dr. Janosz Poha from Ghostbusters 2:

"Everything you are doing is bad...I want you to know this."

Re: Eval This!

2007-08-08 10:55 • by Pilum (unregistered)
148682 in reply to 148679
[quote user="qsdfqsdfsfsdfsqdfffffffffffffff"][quote user="Grant"]What do you think would happen without the eval when PolicyNumber does not exist?!

Didn't anyone tell you that this is the only way to be sure execution proceeds normally when the element doesn't exists?!
[/quote]

False.

if(elementReference) { /* do something */ } // Works nicely in JS.

Re: Eval This!

2007-08-08 10:57 • by jonny s. (unregistered)
I'm not that familiar with scripting languages, what does eval() do?

Re: Eval This!

2007-08-08 10:58 • by Michael (unregistered)
148684 in reply to 148677
SomeCoder:
About a year ago, I inherited a Perl script that was written a lot like this. Basically it had this structure:

eval
{
# Tons of Perl code
}

if ($@) {
# Handle error.
}

He was using it as a try/catch block for Perl. The thing is, the "Tons of Perl code" had plenty of error checking and handling that he could have done for it without the eval and all this monstrosity did was make it nearly impossible to debug.

Won't someone please think of the maintenance programmers?
eval {} is the generally accepted way of providing try/catch functionality in Perl. The only WTF here is the "Tons of Perl code" being evaluated at once.

Re: Eval This!

2007-08-08 11:02 • by whicker (unregistered)
Quite obviously jStr1 through 5 (and 4) are filled in with a Server Side Include mechanism for maximum flexibility and code security. After all, it is against company policy to release source code files.

Re: Eval This!

2007-08-08 11:03 • by qsdfqsdfsfsdfsqdfffffffffffffff (unregistered)
148686 in reply to 148682
[quote user="Pilum"][quote user="qsdfqsdfsfsdfsqdfffffffffffffff"][quote user="Grant"]What do you think would happen without the eval when PolicyNumber does not exist?!

Didn't anyone tell you that this is the only way to be sure execution proceeds normally when the element doesn't exists?!
[/quote]

False.

if(elementReference) { /* do something */ } // Works nicely in JS.[/quote]

One word: sarcasm.

Re: Eval This!

2007-08-08 11:19 • by Brad (unregistered)
Since the code is defined out of order (5 before 4), I am sure that it would never do what was intended, and it would be a very difficult thing to track down.

I guess the person thought that you must use eval to assign variables (he referenced them without the eval just fine).

Re: Eval This!

2007-08-08 11:21 • by awefwafwa (unregistered)
This was pretty common back in the day. It's still pretty common, actually. It's simply people writing code that don't know how to program.

Re: Eval This!

2007-08-08 11:22 • by snoofle (unregistered)
148691 in reply to 148689
Basic: on error resume next
JS: eval(...);eval(...); ...

What else is new?

Re: Eval This!

2007-08-08 11:34 • by dkf (unregistered)
148695 in reply to 148684
Michael:
eval {} is the generally accepted way of providing try/catch functionality in Perl. The only WTF here is the "Tons of Perl code" being evaluated at once.
Not really. This looks like a fallback handler that is used to deal with unexpected errors in a more graceful way (e.g. logging it and reporting "sorry, can't do that" instead of splatting plain text in the middle of something in a different format). In normal operation of such code, nothing ought to trigger the fallback operation.

If it's not being used that way, it's a WTF?! of course.

Re: Eval This!

2007-08-08 11:50 • by Bogglestone (unregistered)

Of course if you use try/catch it is not really error checking. It is error chucking.

My browser already remembers all the captchas...

Re: Eval This!

2007-08-08 12:03 • by Cloak (unregistered)
148700 in reply to 148680
Pilum:
Another big WTF is using document.all to access an element.


getElementById works only since IE6.

Re: Eval This!

2007-08-08 12:04 • by jread
Ok, I don't pretend to know much about Javascript but I thought Eval() just reads text and converts it into script. In that case, why would anyone support what he's doing given that they are "company names"? Does Javascript not recognize strings that are just in quotes?

Re: Eval This!

2007-08-08 12:09 • by abx
148703 in reply to 148676
bstorer:
Hmmm... is that a code injection vulnerability I smell?
Actually, all JS is injection vulnerable. You can redefine any function, add new ones, and execute any code on any website, but, as someone has already pointed out, only on your client.

Re: Eval This!

2007-08-08 12:10 • by makomk
148704 in reply to 148700
Cloak:
Pilum:
Another big WTF is using document.all to access an element.


getElementById works only since IE6.


If anyone's still using IE5.5, that's an even bigger WTF?

(BTW, I could've sworn the RSS feed didn't have ads before...)

Re: Eval This!

2007-08-08 12:27 • by bob ardkor
148706 in reply to 148704
makomk:

If anyone's still using IE5.5, that's an even bigger WTF?


OP:

...some old JavaScript code...



edit : and I don't see any ads in the feed (using netvibes)

Re: Eval This!

2007-08-08 12:28 • by JNeumann (unregistered)
It reminds me of someone who uses Table-Oriented Programming. Where everything is done through evals as the actual 'code' is stored in tables. Not making this up. Look for it.

Re: Eval This!

2007-08-08 12:41 • by bramz (unregistered)
I'm disappointed. I thought the title read "EVIL this!" :(

Re: Eval This!

2007-08-08 12:53 • by nickfitz
148709 in reply to 148700
Cloak:
Pilum:
Another big WTF is using document.all to access an element.


getElementById works only since IE6.


IE has supported getElementById since version 5. That's since March 1999.

Re: Eval This!

2007-08-08 12:54 • by Zygo (unregistered)

companyName = unescape(companyName);
if (window.opener){
jStr1 = "window.opener.document.all.IcompanyName.value = trimString(unescape(companyName))"


OK, I've never written a line of Javascript in my life, but isn't that unescaping companyName twice (thus requiring companyName to look like "Smith &amp;amp; Wesson" or "Coca%252DCola")? Or are the two companyName variables in different scope or something?

Asymmetrical escaping and unescaping always makes me reach for the nearest loaded firearm.

Re: Eval This!

2007-08-08 13:09 • by merreborn
148713 in reply to 148678
durnurd:
bstorer:
Hmmm... is that a code injection vulnerability I smell?


No... not really in that case.

Addendum (2007-08-08 10:52):
Oh, wait, I skimmed over the code, not noticing the CompanyName. So there's a possibility for code injection, but it's Javascript. You'd only be hurting yourself.


Not quite. Ever heard of XSS?

Say, for example, that CompanyName, in this case, is taken from somewhere on the page. Consider also, that perhaps CompanyName is user entered data, and that users can view CompanyNames entered by other users.

A malicious user could then enter a CompanyName with some javascript in it to, say, forward the viewer's session cookie to the malicious user's website, and he could then hijack the user's session. If the hijacked user happens to be some sort of site superuser, then that's just a bonus.

Granted, this requires several layers of vulnerability on top of the eval(), but the moral is simple: don't eval un-filtered user input.

Re: Eval This!

2007-08-08 13:40 • by mrprogguy (unregistered)
148729 in reply to 148702
jread:
Ok, I don't pretend to know much about Javascript but I thought Eval() just reads text and converts it into script. In that case, why would anyone support what he's doing given that they are "company names"? Does Javascript not recognize strings that are just in quotes?


The evil eval() executes whatever script you put into it. The difference between

n = 1;

and

eval("n = 1;")

is that the eval() takes longer.

If I needed to do this sort of thing, I'd make sure window.opener exported functions that could fill its own form fields, so that the child window didn't have to know so much about what was going on upstream.

CAPTCHA: sanitarium (yes, for the person that wrote the original scripting)

Re: Eval This!

2007-08-08 13:44 • by Darien H (unregistered)
148732 in reply to 148703
Main exception: Cross-site scripting, where someone ELSE tinkers with the Javascript on your client, say, by successfully putting some javascript into their sig file on a web forum or something.

They could thus inject code into other people's browsers. For example, the code could scan the page to see if the victim is currently logged in, and then go make fraudulent posts.

That's one of the good reasons to make users confirm major actions (e.g. account password change) with re-entering their password even if they are already logged on.

Re: Eval This!

2007-08-08 13:47 • by Andrew (unregistered)
148734 in reply to 148684
Michael:
SomeCoder:
About a year ago, I inherited a Perl script that was written a lot like this. Basically it had this structure:

eval
{
# Tons of Perl code
}

if ($@) {
# Handle error.
}

He was using it as a try/catch block for Perl. The thing is, the "Tons of Perl code" had plenty of error checking and handling that he could have done for it without the eval and all this monstrosity did was make it nearly impossible to debug.

Won't someone please think of the maintenance programmers?
eval {} is the generally accepted way of providing try/catch functionality in Perl. The only WTF here is the "Tons of Perl code" being evaluated at once.


Perl eval {}; if ($@) {...} catches die(..); events. It should only be used as a try/catch to report run-time errors that can't be checked with if (..) {}, such as network failure.

# No eval: division by zero
if ($denom != 0) { $q = $num/$denon } else { $q = 0; }

# Use eval to catch DBI errors on DB server.
eval {
$dbs = $db->prepare("SELECT * FROM TAB1");
}
if ($@) {
print STDERR "DB server can't prepare SQL\n Reason: $@\n";
}

Notice that $@ reports the DBI error. So, debugging is still quite nice!

Re: Eval This!

2007-08-08 13:59 • by poopdeville
The Real WTF is that the coder used eval() on nearly each line of code, some within a try/catch and others without.

Re: Eval This!

2007-08-08 14:09 • by Foobar (unregistered)
148750 in reply to 148668
s0be:
Normally I can find a way to see what the coder was thinking, but I just can't find ANY justification for this method of doing this.
Payed by line?

Captchs-anti-captchs-remover: ninjas

Re: Eval This!

2007-08-08 14:24 • by Carburizer (unregistered)
I don't know Java, but the coder might have thought of using the evals as a way, however misguided, of abstracting / simplifying the try / catch blocks so they would fit on a single line.

Re: Eval This!

2007-08-08 14:32 • by vt_mruhlin
148760 in reply to 148710
Zygo:

companyName = unescape(companyName);
if (window.opener){
jStr1 = "window.opener.document.all.IcompanyName.value = trimString(unescape(companyName))"


OK, I've never written a line of Javascript in my life, but isn't that unescaping companyName twice (thus requiring companyName to look like "Smith &amp;amp; Wesson" or "Coca%252DCola")? Or are the two companyName variables in different scope or something?

Asymmetrical escaping and unescaping always makes me reach for the nearest loaded firearm.


You're looking at it backwards. escape("Coca-Cola") would return "Coca%2DCola", thus escape(escape("Coca-Cola")) would give "Coca%252DCola"

Unescape reverses the process. So if it was originally "Coca%2DCola", unescaping the first time will change it into "Coca-Cola", and the second time will do nothing.

Re: Eval This!

2007-08-08 14:52 • by Pilum (unregistered)
148763 in reply to 148709
nickfitz:
Cloak:
Pilum:
Another big WTF is using document.all to access an element.


getElementById works only since IE6.


IE has supported getElementById since version 5. That's since March 1999.


The document.forms.elements collections predates both and are still the most reliable and efficient way of accessing forms elements. And that's just two of several collections available.

As it is, getElementById is highly overused.

Re: Eval This!

2007-08-08 15:02 • by Jeff (unregistered)
I had a similar problem with a PHP "program" I inherited from the old webmaster a couple jobs ago. Basically, she would build a string of executable PHP code, save it as a variable, and then eval() it.

"..wait, wait....Joy, why are you doing this?!? You're IN PHP!"

"oh..I guess that makes more sense."

She left 2 days later and I basically had to rewrite the entire thing it was so bad.

Re: Eval This!

2007-08-08 18:59 • by John (unregistered)
//How to call a function in javascript


var lines = my_function.toString().split(/\n/);

for (i=0;i< lines.length;i++) {
try {
eval("try{"+lines[i]+"}catch(e){}");
} catch(e) {
}

}

Re: Eval This!

2007-08-08 19:00 • by Stefan W. (unregistered)
148816 in reply to 148753
Carburizer:
I don't know Java, but the coder might have thought of ...

You don't know Java and you don't know javascript.
This here is javascript.
Java != Javascript - or to speak more on topic:
eval (Java != Javascript) == true.

Re: Eval This!

2007-08-08 22:03 • by zzzxtreme
that's one reason newly employed programmers would want to resign

Re: Eval This!

2007-08-08 22:10 • by zzzxtreme
148845 in reply to 148729
not only that
eval in js is 'global'

function x(){
var a = 1
}

function z(){
eval("var b = 1")
}

the var b will be global

Re: Eval This!

2007-08-08 22:12 • by Dave (unregistered)
I can't believe no-one has noticed the real WTF. This is JavaScript people, not Java. Please press Enter before starting a new block.

Re: Eval This!

2007-08-09 02:32 • by Matt Burgess (unregistered)
document.getElementById has been available a long time, but is iffily implemented in IE up to and including IE6. Specifically, IE will actually retrieve the NAME rather than the ID.

<input type="text" name="name" id="firstName">
<input type="text" name="company_name" id="name">
<script>
alert(document.getElementById('name').value);
</script>

IE and firefox will return different values. Yay. A common trap.

As for the reference to Table-Oriented Programming, I've seen that too. Everything from queries (quite common) to javascript to blocks of PHP stored in various tables for processing. It's a good way to store things because.... um... Nope. I got nothing.

Oh. And yeah. Java != Javascript. That bugs me.

CAPTCHA: seriously... who cares?
« PrevPage 1 | Page 2Next »

Add Comment