I am currently working on adding a fair amount of functionality to a Web site whose programs have been designed very poorly. Amongst other things, taint checking and strict have not been used. Code has been thrown together without regard to side effects, massive Here docs are used to output HTML, etc. Since I am getting a fair amount of experience with these issues, I thought I would offer some of my observations for fellow monks. Some of these pertain to the existing code and concentrates on 'quick fixes'. Some pertains to new code that's added.
Personally, I believe we have an obligation to ensure that our client's code is as secure as possible. Check out Kevin Meltzer's untaint module for a quick and easy way of untainting data. Untainting is necessary, but does not have to be difficult. This fix isn't so quick, but it's mandatory.
If the code uses system calls, using the multiple argument form of system reduces the chance that unsafe data will be passed to the shell and it's often a quick and easy change.
Usually, this will break it, but recently I did this to a program that was 2,000 lines long. I then ran it and the error log had an extra 130 lines in it. While many of the issues were not quick fixes, there were many that were simply a failure to declare variables.
I've discovered that bad code often fails to check to see if open, read, flock, and other system calls were successful. At least add an or die "$!" after them. Nothing is worse than tracking down a bug caused by a silent failure on an open 50 lines earlier.
When using placeholders, the DBI module will automatically quote your data for you. Otherwise, putting the variables directly into the SQL statement could be dangerous. A user entering a single quote mark into a field can be sufficient to crash the program.
# This is bad my $sql = qq{ INSERT INTO ECinterface..CustomContent (contentType, con +tentDate, question, answer) VALUES ('tileInfo', $date, $question, $answer)}; my $sth = $dbh->prepare($sql); $sth->execute; # This is good my $sql = qq{ INSERT INTO ECinterface..CustomContent (contentType, con +tentDate, question, answer) VALUES ( ?,?,?,? )}; my $sth = $dbh->prepare($sql); $sth->execute( 'tileInfo', $date, $question, $answer );
If you can't get the code to run under strict, make sure that when you build new functionality, that you at least use strict on the code you have created.
Code reuse is good, but not if the original code is junk. If you are writing a sub similar to one that already exists, consider not updating the existing code. Rewrite the function from scratch, allowing calls to the original subroutine, method, or whatever, to be routed to and handled by your code. Then, delete the original code, if possible.
I recently spent half an hour not seeing a misspelled variable because "use strict" was not in place. It would have taken me a couple of seconds to find the bug, otherwise. By pointing out how much extra time I spend maintaining code as a result of poor design, I find that my boss is much more willing to give me leeway on deadlines.
Often, poor code is not commented well. Do not add to the problem. Further, if some programmer comes behind you and is trying to figure out why your code is structured so differently, they'll appreciate the heads up. Sometimes I need to do some strange tricks to add my sauce to the spaghetti. Further, add a "to do" list comment at the top of the code so that you and others won't forget.
Any and all tips that others wish to add are welcome!
Cheers,
Ovid
Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.
In reply to Suggestions for working with poor code by Ovid
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |