Bug #729
closedunqualified names used for shell commands.
0%
Description
There are several places in the appion where the python subprocess function is used to call shell commands like 'cp'. These calls are not qualified (they don't use the absolute path to the program like /bin/cp). This poses a potential problem if the command doesn't behave as expected. For instance if cp is aliased to 'cp -i' then the cp command will wait indefinitely for user input if the file already exists. These commands should all use an absolute path to the command such as '/bin/cp'
Updated by Neil Voss over 14 years ago
I disagree what if my cp command is in /usr/local/bin/ or something. Ideally, one should use shutil.copy(src, dest) in your example.
If the user wants cp -i, the should check if its a login shell or something.
Updated by Christopher Irving over 14 years ago
I agree, ideally shutil.copy should be used and is what I original suggested. However, Jim and Anchi feel that this is not the ideal case and that we should still call subprocess. In that case, we should use a fully qualified command name. What if the program you have in /usr/local/bin/cp isn't even the copy command. Unless some has seriously altered their system in ways they shouldn't /bin/cp is as universal as you are going to get. Using it makes sure you getting the expected behavior.
Updated by Anchi Cheng over 14 years ago
While shutil.copy(src, dest) will solve the calls from python, I saw a few places that it is part of a series of commands written to a file and then executed. What would we do there then?
Updated by Jim Pulokas over 14 years ago
I will start to go through and fix any of these problems. I assume we are worried about any command execution, not just simple shell commands like "cp"? For example, there are calls to "tar", "svn", and others. Some of them are probably safe, but I guess we can't assume anything about how someone configured their system.
This is probably a problem throughout all of myami, not just appion.
The first step is to find all places where this could be a problem. Although suprocess.Popen is the currently accepted way of calling external programs, there are other older functions to look for, like os.popen, os.system, os.exec*. Here is a command to locate potential lines to fix, executed in the top level of myami:
grep -rI --exclude-dir=.svn "Popen" *
or replace "Popen" with "os.system" "popen" or whatever. There are around 300 lines returned. In some cases, the line is something like "Popen(cmd...)" which means you have to dig into the preceding lines to see if cmd is unsafe.
Then there are the places like Anchi mentions, where a script or job file is being built and then executed. This applies to php and python. It's hard to grep for those.
Updated by Amber Herold over 14 years ago
- Target version changed from Appion/Leginon 2.0.2 to Appion/Leginon 2.1.0
Updated by Jim Pulokas over 14 years ago
- Status changed from Assigned to In Code Review
- Assignee changed from Jim Pulokas to Christopher Irving
As discussed at the appion meeting, the priority here is to change the three commands "cp", "rm", "mv", since those are the most commonly modified through aliases. I have run some grep/sed batch processing through all of myami to find and correct these cases. I identified and corrected only the following regular expressions, which find those three commands if they are preceded by a single or double quote:
['"]cp\W ['"]mv\W ['"]rm\W
I have no idea if that covers everything, but it does find false positives in the following files which had to be reverted:
myamiweb/project/project.php myamiweb/project/user.php myamiweb/inc/leginon.inc myamiweb/processing/inc/particledata.inc
All of these changes have been commited as r14819.
This should be good enough for this release. Then we should proceed with Neil's suggestion for the next release, using python methods rather than shell commands where possible. A lot of it is in php, so maybe there are php methods as well.
Updated by Christopher Irving about 14 years ago
- Status changed from In Code Review to In Test
Reviewed, looks good to me.
Updated by Christopher Irving about 14 years ago
- Status changed from In Test to Closed
I've tested all the php code as best I could with really knowing appion very well. I validated the python code using an interactive python shell.