How bad can you make this code

Ye' old general discussion board. Basically, for everything that isn't covered elsewhere. Come here to shoot the breeze, shoot your mouth off, or whatever suits your fancy.
This forum is not for asking programming related questions.

Moderator: General Moderators

Post Reply
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

How bad can you make this code

Post by Ollie Saunders »

Here's a typical code snippet. Does any body have any tips on how I could make it worse? I've already done my best, I mean worst.

Code: Select all

<body marginheight=10>
<?include 'stuff.inc';

mysql_connect ("localhost", "root", "password");
mysql_select_db ("img"); extract ($_REQUEST);
    /* performs a mysql query */

    $r = mysql_query ("select * from pict where artist=\'".$txtAuthorId."\'");
$r2 = mysql_num_rows ($r);
 if ($r2) {
echo mysql_num_rows ($r) . ' found:<br /><br />';
echo '<ul>';
while ($row = mysql_fetch_row ($r)) echo "<li>$row[0]</li>"; echo "</ul>";
}
else  header ("location:home.php");?></body>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

what are you trying to accomplish with this demonstration?
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Post by alex.barylski »

Probably demonstrating how annoying it is when you stumble across code thats just junk... :P
Charles256
DevNet Resident
Posts: 1375
Joined: Fri Sep 16, 2005 9:06 pm

Post by Charles256 »

damn.. I had to look up extract. LOL
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Use unbraced ifs that are indented as if there should be more than one statement.

Edit - W00t 2000 posts!
Last edited by Ambush Commander on Sun Aug 13, 2006 3:06 pm, edited 1 time in total.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

damn.. I had to look up extract. LOL
Once upon a time I used to use extract a lot! But i realised, hang on this this is <span style='color:blue' title='I&#39;m naughty, are you naughty?'>smurf</span>.
Use unbraced ifs that are indented as if there should be more than one statement.
Will do thanks.
what are you trying to accomplish with this demonstration?
Only time will tell ;)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

I added a header injection and database connection vuneribilities too :)

Code: Select all

<body marginheight=10>
<?include 'stuff.inc';

extract ($_REQUEST);
mysql_connect ($data, "root", "password");
 mysql_select_db ("img"); 
/* performs a mysql query */

$r = mysql_query ("select * from pict where artist=\'".$txtAuthorId."\'");
$r2 = mysql_num_rows ($r);
if ($r2) 
    echo mysql_num_rows ($r) . ' found:<br /><br />';
    echo '<ul>';
    while ($tmp = mysql_fetch_row ($r)) echo "<li>$tmp[0]</li>"; echo "</ul>";
else  header ("location:$location");?></body>
Edit:
AC wrote:Edit - W00t 2000 posts!
w00t! well done AC :)
Charles256
DevNet Resident
Posts: 1375
Joined: Fri Sep 16, 2005 9:06 pm

Post by Charles256 »

wow..when did I past 1k posts? I remember waiting for that benchmark..and then I didn't even notice..figures :(
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

I think you did it a couple of days ago. I remember thinking that I wanted to PM you a congrats, but I must have gotten wrapped up upping my post count. Oh well, I guess it'll just have to wait for 2000 posts. Let's see, is there anyone with 200 posts I can congratulate... hmm... Oh yeah! Ambush....

Yeah AC, 2000 posts! W00t, good job man!

@: Ole
As for making your code worse, how about letting the user choose the database table. And how about a call to session_start() or setcookie() after the <body> HTML output? Now that's some bad stuff. Or comparisons with undeclared vars, hmmm.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

He guys, what about this: after this piece of code cannot be made any worse, why not make it better again? Step by step, to show everyone who doesn't see why the code is bad, how it can be better? And why? Refactoring seems a popular word here .. :)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

As for making your code worse, how about letting the user choose the database table. And how about a call to session_start() or setcookie() after the <body> HTML output? Now that's some bad stuff. Or comparisons with undeclared vars, hmmm.
Too late now. I did take off a semicolon on the mysql_connect and remove '</li>' on the penultimate line though.
He guys, what about this: after this piece of code cannot be made any worse, why not make it better again? Step by step, to show everyone who doesn't see why the code is bad, how it can be better? And why? Refactoring seems a popular word here .. Smile
You're spoiling the surprise, cause that's what I'm going to do. But not for quite a while. But to keep you happy for now, how many unique items for improvement can you find?

Code: Select all

<body marginheight=10>
<?include 'stuff.inc';

extract ($_REQUEST);
mysql_connect ("$data", "root", "password")
 mysql_select_db ("img");
/* performs a mysql query */

$r = mysql_query ("select * from pict where artist=\'".$txtAuthorId."\'");
$r2 = mysql_num_rows ($r);
if ($r2)
    echo mysql_num_rows ($r) . " found:<br /><br />";
    echo '<ul>';
    while ($tmp = mysql_fetch_row ($r)) echo "<li>$tmp[0]"; echo "</ul>";
else  header ("location:$location");?></body>
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

1. Body tag uses "marginheight"; a non-standard attribute
2. Value for the attribute isn't quoted
3. PHP code block begins after output
4. PHP block opened with short-tag
5. include used to get required variables
6. File requested with include is named poorly
7. File requested with include has INC extension; exposure vulnerability
8. Use of extract function
9. Function call and parenthesis separated by space
10. Use of dollar underscore request
11. Database connection vulnerability
12. Redundant double quotes around variable name
13. Strings not making use of any double quote functionally are nonetheless double quoted
14. Visible password
15. Very weak password
16. Missing semi-colon after mysql connect call
17. Database connection is assumed to be successful
18. Database selection is assumed to be successful
19. Unclear as to what the comment is referring to
20. Should you realise that it the comment referring to the call to mysql query you will notice the comment is utterly superfluous
21. Dollar R is a cryptic, non-conventional variable name
22. SQL query entered directly into mysql query call
23. SQL query keywords in lowercase
24. Origin of dollar text author id unknown (this is because of that extract dollar underscore request call some lines back); SQL injection vulnerability.
25. Dollar text author id not escaped
26. Dollar R 2; variable name ending in a number
27. If control structure's condition is checking for truth on a variable containing an integer; that's only minor
28. If control structure is missing braces
29. Repeat call of already stored data
30. Use of mysql fetch row when mysql fetch assoc or similar would be preferred
31. Array subscript used without braces inside a double quoted string
32. TMP zero has not been HTML escaped before being output; cross site scripting vulnerability
33. List item tag not closed
34. Unnumbered list tag is closed but the echo statement appears on the same line as a while yet not inside it
35. lowercase L for location in header call
36. Missing space between location colon and value
37. Header injection vulnerability
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

I'll try to answer your question and see how many I come up with without looking secretly at the answer, ok :)
Post Reply