graik / biskit

A Python platform for Structural Bioinformatics
http://biskit.pasteur.fr
Other
54 stars 20 forks source link

Use of deprecated mktemp #24

Closed suliat16 closed 6 years ago

suliat16 commented 6 years ago

According to the documentation: Use of this function may introduce a security hole in your program. By the time you get around to doing anything with the file name it returns, someone else may have beaten you to the punch. mktemp() usage can be replaced easily with NamedTemporaryFile(), passing it the delete=False parameter. Thus far, I've spotted mktemp() in executor.py.

graik commented 6 years ago

To be fair, it is very unlikely to have anyone else generating the same temporary name since we always put a prefix and suffix there. Issues could only arise if parallel processes of a biskit program are accessing the same file system (again, that's usually not the case) but even then the random generated name makes name conflicts extremely unlikely. In fact, many of these wrappers have seen months of computation time on computer clusters without this ever becoming an issue.

tempfile.mktemp is used a lot throughout biskit. I counted 119 uses. Here is the problem: All the proposed alternatives are creating the (empty) file right away. Most of the time, this is exactly not what we want. We only want to generate the file name which is then passed on to the external program as a parameter, telling e.g. what the name of the output file is supposed to be. Many programs refuse to override an output file that already exists.

So mktemp is perfect for our purposes and replacing it would create a whole new set of problems. I rather stick to it until it is actually removed from tempfile. Closing the issue but thanks for the heads up!