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

crypt.h should be in _cryptmodule.c, not in public header #88914

Closed
geofft mannequin opened this issue Jul 27, 2021 · 8 comments
Closed

crypt.h should be in _cryptmodule.c, not in public header #88914

geofft mannequin opened this issue Jul 27, 2021 · 8 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-C-API

Comments

@geofft
Copy link
Mannequin

geofft mannequin commented Jul 27, 2021

BPO 44751
Nosy @vstinner, @miss-islington, @geofft, @thesamesam
PRs
  • closes bpo-44751: Move crypt.h include from public header to _cryptmodule #27394
  • [3.10] closes bpo-44751: Move crypt.h include from public header to _cryptmodule (GH-27394) #28636
  • [3.9] closes bpo-44751: Move crypt.h include from public header to _cryptmodule (GH-27394) #28638
  • 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 2021-07-27.22:58:40.724>
    created_at = <Date 2021-07-27.21:23:37.808>
    labels = ['3.8', '3.9', '3.10', '3.11', 'expert-C-API', '3.7']
    title = 'crypt.h should be in _cryptmodule.c, not in public header'
    updated_at = <Date 2021-09-30.21:54:16.845>
    user = 'https://github.com/geofft'

    bugs.python.org fields:

    activity = <Date 2021-09-30.21:54:16.845>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-27.22:58:40.724>
    closer = 'miss-islington'
    components = ['C API']
    creation = <Date 2021-07-27.21:23:37.808>
    creator = 'geofft'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44751
    keywords = ['patch']
    message_count = 8.0
    messages = ['398321', '398328', '402894', '402909', '402919', '402920', '402986', '402990']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'Arfrever', 'miss-islington', 'geofft', 'thesamesam']
    pr_nums = ['27394', '28636', '28638']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue44751'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @geofft
    Copy link
    Mannequin Author

    geofft mannequin commented Jul 27, 2021

    In bpo-32635, it was discovered that _cryptmodule.c was missing a dependency on crypt.h, which caused it to segfault when it was missing the proper prototype for crypt. This was fixed by adding an #include <crypt.h> to Python.h.

    This include doesn't need to be in the public header; it only needs to be in _cryptmodule.c. Removing it from the public header is helpful for packagers, because it means that the libpython-dev (or whatever) package doesn't need a dependency on libcrypt-dev, only on the libcrypt runtime library.

    @geofft geofft mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-C-API labels Jul 27, 2021
    @miss-islington
    Copy link
    Contributor

    New changeset 196998e by Geoffrey Thomas in branch 'main':
    closes bpo-44751: Move crypt.h include from public header to _cryptmodule (GH-27394)
    196998e

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Sep 29, 2021

    Could you backport this fix to at least 3.9 and 3.10 branches?
    This is fix for regression which was previously backported to 2.7 and 3.6 branches.

    @miss-islington
    Copy link
    Contributor

    New changeset 9626ac8 by Miss Islington (bot) in branch '3.9':
    closes bpo-44751: Move crypt.h include from public header to _cryptmodule (GH-27394)
    9626ac8

    @vstinner
    Copy link
    Member

    New changeset 80285ec by Miss Islington (bot) in branch '3.10':
    closes bpo-44751: Move crypt.h include from public header to _cryptmodule (GH-27394) (GH-28636)
    80285ec

    @vstinner
    Copy link
    Member

    Could you backport this fix to at least 3.9 and 3.10 branches?

    Done.

    This is fix for regression which was previously backported to 2.7 and 3.6 branches.

    I'm not sure what you mean.

    In Python 2.7 and Python 3.6, <crypt.h> was already included by Include/Python.h:

    #ifdef HAVE_CRYPT_H
    #include <crypt.h>
    #endif

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Sep 30, 2021

    > This is fix for regression which was previously backported to 2.7 and 3.6 branches.

    I'm not sure what you mean.

    In Python 2.7 and Python 3.6, <crypt.h> was already included by Include/Python.h:

    See issue bpo-32635, where Victor Stinner backported some commit (with problematic location of '#include <crypt.h>') to 2.7 and 3.6 branches (which was released in >=2.7.15 and >=3.6).

    @vstinner
    Copy link
    Member

    See issue bpo-32635, where Victor Stinner backported some commit (with problematic location of '#include <crypt.h>') to 2.7 and 3.6 branches (which was released in >=2.7.15 and >=3.6).

    crypt.h was added to Python.h by: https://github.com/python/cpython/pull/5284/files

    And this change was then backported to 2.7 and 3.6. Ok, now I understand.

    Well, it's now fixed in all branches accepting bugfixes.

    @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
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-C-API
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants