Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use thread-safe functions instead of unsafe ones (crypt, ttyname) #80342

Closed
Yhg1s opened this issue Mar 1, 2019 · 5 comments
Closed

Use thread-safe functions instead of unsafe ones (crypt, ttyname) #80342

Yhg1s opened this issue Mar 1, 2019 · 5 comments
Labels
easy interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@Yhg1s
Copy link
Member

Yhg1s commented Mar 1, 2019

BPO 36161
Nosy @Yhg1s, @tiran, @benjaminp, @vadmium, @Mariatta, @csabella
PRs
  • bpo-36161: Use thread-safe function ttyname_r instead of unsafe one ttyname #14868
  • closes bpo-38402: Check error of primitive crypt/crypt_r. #16599
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-10-09.02:19:56.293>
    created_at = <Date 2019-03-01.21:30:26.143>
    labels = ['interpreter-core', 'easy']
    title = 'Use thread-safe functions instead of unsafe ones (crypt, ttyname)'
    updated_at = <Date 2019-10-09.02:19:56.289>
    user = 'https://github.com/Yhg1s'

    bugs.python.org fields:

    activity = <Date 2019-10-09.02:19:56.289>
    actor = 'benjamin.peterson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-10-09.02:19:56.293>
    closer = 'benjamin.peterson'
    components = ['Interpreter Core']
    creation = <Date 2019-03-01.21:30:26.143>
    creator = 'twouters'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36161
    keywords = ['patch', 'easy (C)']
    message_count = 5.0
    messages = ['336957', '336962', '336963', '351981', '354244']
    nosy_count = 6.0
    nosy_names = ['twouters', 'christian.heimes', 'benjamin.peterson', 'martin.panter', 'Mariatta', 'cheryl.sabella']
    pr_nums = ['14868', '16599']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue36161'
    versions = []

    @Yhg1s
    Copy link
    Member Author

    Yhg1s commented Mar 1, 2019

    (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.

    @Yhg1s Yhg1s added interpreter-core (Objects, Python, Grammar, and Parser dirs) easy labels Mar 1, 2019
    @vadmium
    Copy link
    Member

    vadmium commented Mar 1, 2019

    In bpo-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)”?

    @Yhg1s
    Copy link
    Member Author

    Yhg1s commented Mar 1, 2019

    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).

    @tiran
    Copy link
    Member

    tiran commented Sep 11, 2019

    Does it make sense to modify crypt at all? PEP-594 is going to deprecate and remove crypt soon.

    @benjaminp
    Copy link
    Contributor

    New changeset 594e2ed by Benjamin Peterson (Antonio Gutierrez) in branch 'master':
    closes bpo-36161: Use thread-safe ttyname_r instead of ttyname. (GH-14868)
    594e2ed

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants