classification
Title: Use thread-safe functions instead of unsafe ones (crypt, ttyname)
Type: Stage: resolved
Components: Interpreter Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mariatta, benjamin.peterson, cheryl.sabella, christian.heimes, martin.panter, twouters
Priority: normal Keywords: easy (C), patch

Created on 2019-03-01 21:30 by twouters, last changed 2019-10-09 02:19 by benjamin.peterson. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 14868 merged chibby0ne, 2019-07-20 00:58
PR 16599 open chibby0ne, 2019-10-05 19:34
Messages (5)
msg336957 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2019-03-01 21:30
(good first issue, I think.)

CPython currently uses a few functions that are thread-unsafe where thread-safe version exist, at least in glibc (crypt() and ttyname(), at least). This isn't an issue when looking at CPython in isolation because these calls are guarded by the GIL, but it can be a problem if the C functions are called from other code (in an extension module, or from an application embedding CPython).

I expect it to be uncommon to be the case with crypt() and ttyname(), so it's probably not important to fix, but I do think these would make good first issues. crypt(), in particular, is called from a very self-contained (and otherwise) module, and could do with some other enhancements (proper error handling, properly #including crypt.h).

I suggest a new contributor (or someone new to C) take this on and do something like the following, using crypt as an example:
 - Add configure.ac checks for crypt.h and crypt_r()
 - In Modules/_cryptmodule.c, if present, import crypt.h and use crypt_r.
 - Check for a NULL return and raise OSerror, but possibly only when using crypt_r() (for compatibility with systems where crypt() doesn't do the right thing on error, perhaps).
 - Add tests for the new error handling (crypt() on glibc will return an error on invalid salts).

For ttyname() (called from Modules/posixmodule.c), the change would be simpler (it already handles errors), but a choice will have to be made about the size of the buffer to use, and whether to use a static buffer (which would be protected by the GIL).

There may be other functions being used in posixmodule or other modules that could conflict with non-CPython uses of the thread-unsafe functions that we could switch to thread-safe versions, but other than manually looking I'm not sure yet how to find them.
msg336962 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2019-03-01 22:31
In Issue 28503, “crypt_r” was added to Python 3.7 and 3.8+, and it looks like it is still there.

Regarding error handling for “crypt”, it is not documented, but the Python function returns None on error. You would have to consider backwards compatibility to use OSError. Perhaps add a new parameter like “crypt(raise_error=True)”?
msg336963 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2019-03-01 22:36
Ah, looks like I missed crypt_r getting added in 3.7. Sorry about that. Yes, the error handling would be a behaviour change, a keyword argument may be a good solution. As it is, crypt() returns None, throwing away the *reason* for the failure (which is recorded in errno).
msg351981 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-09-11 16:54
Does it make sense to modify crypt at all? PEP 594 is going to deprecate and remove crypt soon.
msg354244 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2019-10-09 02:19
New changeset 594e2edfb5e0d24e03469d035d8f39ff29a64d99 by Benjamin Peterson (Antonio Gutierrez) in branch 'master':
closes bpo-36161: Use thread-safe ttyname_r instead of ttyname. (GH-14868)
https://github.com/python/cpython/commit/594e2edfb5e0d24e03469d035d8f39ff29a64d99
History
Date User Action Args
2019-10-09 02:19:56benjamin.petersonsetstatus: open -> closed

nosy: + benjamin.peterson
messages: + msg354244

resolution: fixed
stage: patch review -> resolved
2019-10-05 19:34:39chibby0nesetpull_requests: + pull_request16187
2019-09-11 16:54:37christian.heimessetnosy: + christian.heimes
messages: + msg351981
2019-07-20 00:58:47chibby0nesetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request14655
2019-03-01 22:36:26twouterssetmessages: + msg336963
2019-03-01 22:31:38martin.pantersetnosy: + martin.panter
messages: + msg336962
2019-03-01 21:30:26twouterscreate