Re: Bad code from the trenches

Another few problems:

How big is FILE? Why are you reading it completely into RAM before using it?
Fix: Iterate over FILE in a while-loop.
Are you sure you reading it completely before someone else modifies it? Why aren't you flock'ing the filehandle? You are running in an environment where the same script could be trying to open the same file more than once and /or another script potentially could be changing the file at the same time.
Fix: Use flock.
Are you checking for file existence first? What about if the path is a directory?
Fix: Use -d and -e (and -f, potentially)
Are your pathnames relative to server root? What happens if your user specifies '/etc/passwd' or '/proc/cpuinfo' or some other file they really shouldn't be touching?
Fix: Don't allow your user to specify a filename. Make them choose from a list, then verify they actually chose from that list.

Alternatives to your fixes above:

You correctly specify the open() fixes for Perls greater than 5.005_03 (when 3-arg open was introduced). sysopen() is always a fix for security holes. perlopentut has the details.
You're right in that you don't need double-quotes and that does skirt the immediate issue. However, the correct solution is to force your user to pick from a list of acceptable files and make sure they picked from it. (q.v. problems list above.) won't work as expected ...

Being right, does not endow the right to be rude; politeness costs nothing.
Being unknowing, is not the same as being stupid.
Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

Comment on Re: Bad code from the trenches

Replies are listed 'Best First'.
Re^2: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 14:27 UTC
First off, I feel the need to be a little defensive and say "This ain't my code!" It was in a module written by the head developer of my current client. Ok, with that out of the way.... Your suggestion about picking from a list of acceptable files is a good one, but wouldn't work in this case (because of information that was not given in the original post). This code was used in a CGI environment and $dest was derived from a CGI param, but this wasn't code that was directly user-interactive, it was called from a PHP script. As to testing files with -e, -f, and -d before opening them...remember that doing this: `line 1) if ( -e $file && -f $file && -s $file < $ok ) { line 2) ...open file here.... }` [download] means that you have a race condition. If someone modifies or deletes $file in the moment between lines 1 and 2, the tests were all for nothing. A better solution is to do something like this (almost directly lifted from perlopentut): `use 5.004; use Fcntl qw(:DEFAULT :flock); open(FH, '<', $file) or die "can't open $file: $!"; unless (flock(FH, LOCK_SH \| LOCK_NB)) { local $\| = 1; print "Waiting for lock..."; flock(FH, LOCK_SH) or die "can't lock $file: $!"; print "got it.\n" } # now read from FH` [download]	[reply] [d/l] [select]
Re^3: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 14:36 UTC
The problem is that not all OSes allow you to treat a directory as a normal file. Unix-like systems (including OS-X, but not OS-9) do, but Windows probably won't and VMS definitely won't (AFAIK). What happens then? As for the code being yours or not ... don't feel defensive. If it's not yours, you don't have anything to worry about. If it is yours, you've taken the right step(s) to fix your error(s) and learn better programming techniques. Either way, you have no fault. As for the entire pick-a-file thing ... there should still be a list of permissible files being presented within the PHP. At that point, both the Perl and the PHP need to agree on what this list of files is so that the PHP can present the list and the Perl can validate against it. Just because you have more than one code file doesn't mean the solution is wrong. Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^4: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 20:23 UTC
As for the entire pick-a-file thing ... there should still be a list of permissible files being presented within the PHP. At that point, both the Perl and the PHP need to agree on what this list of files is so that the PHP can present the list and the Perl can validate against it. Just because you have more than one code file doesn't mean the solution is wrong. Sorry, I was unclear. It certainly doesn't make it wrong, but I don't have control over the PHP files, so it isn't a solution I can implement. For someone else, who is taking this discussion as advice on how to write a green-field project, it would certainly be a good way to go.	[reply]
Re^2: Bad code from the trenches by itub (Priest) on Mar 17, 2005 at 17:49 UTC
How big is FILE? Why are you reading it completely into RAM before using it? Fix: Iterate over FILE in a while-loop. And that only works if the file has reasonably long lines. You are still in trouble if they give you '/dev/zero' as the filename. Even if the files can not be picked from a list, at least the program should ensure that they are located in a "reasonable" directory.	[reply]
Re^3: Bad code from the trenches by dragonchild (Archbishop) on Mar 18, 2005 at 13:57 UTC
Define "reasonable" in a platform-independent way. Or, is this something that should be included in File::Spec? Who gets to make that list? In other words, /dev is perfectly reasonable on Win32 and /Windows is perfectly reasonable on Linux. Now what? Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^4: Bad code from the trenches by itub (Priest) on Mar 18, 2005 at 20:37 UTC
Who says it must be platform independent? Not every script ever written needs to be portable. What is reasonable depends on the application, so I don't think it's something you can put on File::Spec. For example, if you are writing a CGI that is supposed to serve a file to the user, you can probably have all such files confined to a specific directory, and allow only relative filenames that don't contain double dots. To be on the safe side, I would constrain the filename to the smallest possible set of characters, such as /^\w+$/. But again, it depends on the application.	[reply]