Don't use mktemp #41
Labels
Comments
|
Thanks a lot for the note. Do you know an alternative? If so, maybe one could contribute a PR. |
|
Every other function in tempfile is good.
…On Fri, 21 Dec 2018 at 11:36, Sebastian Thiel ***@***.***> wrote:
Thanks a lot for the note. Do you know an alternative? If so, maybe one
could contribute a PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB-iqZo9xEUwEki27A5R20hcxbGfAJ8ks5u7MemgaJpZM4YwxDn>
.
|
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Dec 13, 2023
The tempfile.mktemp function is deprecated, because of a race condition where the file may be concurrently created between when its name is generated and when it is opened. Other faciliies in the tempfile module overcome this by generating a name, attempting to create the file or directory in a way that guarantees failure if it already existed, and, in the occasional case that it did already exist, generating another name and trying again (stopping after a predefined limit). For further information on mktemp deprecation: - https://docs.python.org/3/library/tempfile.html#tempfile.mktemp - gitpython-developers/smmap#41 The security risk of calls to mktemp in this project's test suite is low. However, it is still best to avoid using it, because it is deprecated, because it is (at least slightly) brittle, and because any use of mktemp looks like a potential security risk and thereby imposes a burden on working with the code (which could potentially be addressed with detailed comments analyzing why it is believed safe in particular cases, but this would typically be more verbose, and at least as challenging to add, as replacing mktemp with a better alternative). This commit replaces *some* uses of mktemp in the test suite: those where it is readily clear how to do so in a way that preserves the code's intent: - Where a name for a temporary directory is generated with mktemp and os.mkdir is called immediately, mkdtemp is now used. - Where a name for a temporary file that is not customized (such as with a prefix) is generated with mktemp, such that the code under test never uses the filename but only the already-open file-like object, TemporaryFile is now used. As the name isn't customized, the test code in these cases does not express an intent to allow the developer to inspect the file after a test failure, so even if the file wasn't guaranteed to be deleted with a finally block or context manager, it is fine to do so. TemporaryFile supports this use case well on all systems including Windows, and automatically deletes the file. - Where a name for a temporary file that *is* customized (such as with a prefix) to reflect the way the test uses it is generated with mktemp, and the test code does not attempt deterministic deletion of the file when an exception would make the test fail, NamedTemporaryFile with delete=False is now used. The original code to remove the file when the test succeeds is modified accordingly to do the same job, and also commented to explain that it is being handled this way to allow the file to be kept and examined when a test failure occurs. Other cases in the test suite should also be feasible to replace, but are left alone for now. Some of them are ambiguous in their intent, with respect to whether the file should be retained after a test failure. Others appear deliberately to avoid creating a file or directory where the code under test should do so, possibly to check that this is done properly. (One way to preserve that latter behavior, while avoiding the weakness of using mktemp and also avoiding inadverently reproducing that weakness by other means, could be to use a path in a temporary directory made for the test.) This commit also doesn't address the one use of mktemp in the code under test (i.e., outside the test suite, inside the git module).
This was referenced Dec 13, 2023
EliahKagan
added a commit
to EliahKagan/smmap
that referenced
this issue
Dec 13, 2023
This uses NamedTemporaryFile with delete=False to replace the one use of the deprecated mktemp function in smmap (reported in gitpython-developers#41). This avoids the race condition inherent to mktemp, as the file is named and created together in a way that is effectively atomic. Because NamedTemporaryFile is not being used to automatically delete the file, it use and cleanup are unaffected by the change.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Security scanners like Bandit don't like anyone using mktemp (smmap/test/lib.py), so to silence them consider using other functions in tempfile.
The text was updated successfully, but these errors were encountered: