Skip to content

closes bpo-44751: Move crypt.h include from public header to _cryptmodule #27394

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

Merged
merged 2 commits into from
Jul 27, 2021

Conversation

geofft
Copy link
Contributor

@geofft geofft commented Jul 27, 2021

https://bugs.python.org/issue44751

Automerge-Triggered-By: GH:benjaminp

@@ -4,6 +4,9 @@
#include "Python.h"

#include <sys/types.h>
#ifdef HAVE_CRYPT_H
#include <crypt.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to #define _GNU_SOURCE here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no because it's defined by pyconfig.h which is included by Python.h. I'm going to try a build on my Linux box to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compiled Python 3.7 on my Debian 9 box with this patch (because that was easiest), and crypt.crypt() works without segfaulting and links crypt_r and there are no warnings in the build log. From inspection, pyconfig.h does define _GNU_SOURCE and does get pulled in by Python.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, great.

@benjaminp benjaminp changed the title bpo-44751: Move crypt.h include from public header to _cryptmodule closes bpo-44751: Move crypt.h include from public header to _cryptmodule Jul 27, 2021
@miss-islington
Copy link
Contributor

@geofft: Status check is done, and it's a success ❌ .

@miss-islington
Copy link
Contributor

@geofft: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: 3 of 5 required status checks have not succeeded: 2 expected..

@miss-islington
Copy link
Contributor

@geofft: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 196998e into python:main Jul 27, 2021
@geofft geofft deleted the crypt-public-header branch July 28, 2021 00:29
@vstinner
Copy link
Member

vstinner commented Aug 4, 2021

Thank you! I'm not sure why <crypt.h> was in Python.h initially. Likely because it was easier to put it here.

fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Aug 17, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Aug 18, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Aug 18, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Sep 23, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
@Arfrever
Copy link
Contributor

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.

@vstinner vstinner added the needs backport to 3.10 only security fixes label Sep 29, 2021
@miss-islington
Copy link
Contributor

Thanks @geofft for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-28636 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 29, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 29, 2021
…dule (pythonGH-27394)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)

Co-authored-by: Geoffrey Thomas <[email protected]>
@vstinner vstinner added the needs backport to 3.9 only security fixes label Sep 29, 2021
@miss-islington
Copy link
Contributor

Thanks @geofft for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Sep 29, 2021
@bedevere-bot
Copy link

GH-28638 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 29, 2021
…dule (pythonGH-27394)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)

Co-authored-by: Geoffrey Thomas <[email protected]>
miss-islington added a commit that referenced this pull request Sep 29, 2021
…dule (GH-27394)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)

Co-authored-by: Geoffrey Thomas <[email protected]>
vstinner pushed a commit that referenced this pull request Sep 29, 2021
…dule (GH-27394) (GH-28636)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)

Co-authored-by: Geoffrey Thomas <[email protected]>
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Oct 19, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Oct 19, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Oct 24, 2021
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
@lazka
Copy link
Contributor

lazka commented Dec 28, 2021

crypt is sadly still needed for packages using python-config:

python3.9-config --libs
 -lcrypt -lpthread -ldl  -lutil -lm -lm

Not sure why python-config leaks all those internals in the first place.

@thesamesam
Copy link
Contributor

thesamesam commented Dec 28, 2021

crypt is sadly still needed for packages using python-config:

python3.9-config --libs
 -lcrypt -lpthread -ldl  -lutil -lm -lm

Not sure why python-config leaks all those internals in the first place.

I think that was fixed separately in #28881 (but not backported). But agreed, I'm not really sure either, and it leads to a lot of applications linking unnecessarily against extra bits.

This whole thing bit us quite hard downstream when changing to libxcrypt and getting build orders right during the migration, as things were requesting it when they didn't actually need it.

@lazka
Copy link
Contributor

lazka commented Dec 28, 2021

Ah, thanks. I'm going to backport that then.

Yeah, I guess for static builds it makes sense the way it is, but usually that is hidden behind a "--static" flag.

@thesamesam
Copy link
Contributor

thesamesam commented Dec 28, 2021

Ah, thanks. I'm going to backport that then.

Yeah, I guess for static builds it makes sense the way it is, but usually that is hidden behind a "--static" flag.

FWIW, we've backported it in Gentoo safely and nothing exploded, so it should be okay for MSYS2 too.

What I think we really need/want is for packages to start using pkg-config for this, which does The Right Thing. I've considered making python-config fatal for some tests runs locally to try weed them out, but worried about the huge amount of work.

pkg-config --libs python-3.10 vs pkg-config --libs python-3.10 --static does what you'd expect.

@vstinner
Copy link
Member

vstinner commented Jan 7, 2022

This PR is closed. If there is still a bug, please open a new issue at bugs.python.org ;-)

@MarshalX
Copy link

Will it be backported to 3.8 and 3.7?

@Arfrever
Copy link
Contributor

Those branches accept almost only security fixes, so unfortunately probably no.

@vstinner
Copy link
Member

Will it be backported to 3.8 and 3.7?

No, you have to backport this change downstream manually (yourself). Python 3.7 and 3.8 no longer accept bugfixes: https://devguide.python.org/#status-of-python-branches

joseluismarti pushed a commit to joseluismarti/xbmc that referenced this pull request Apr 15, 2022
backport python/cpython#27394 to 3.9.6
Android Pycryptodome builds fail due to crypt.h inclusion (not respecting undef HAVE_CRYPT_H)
gentoo-bot pushed a commit to gentoo/cpython that referenced this pull request May 21, 2024
…dule (pythonGH-27394)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)
gentoo-bot pushed a commit to gentoo/cpython that referenced this pull request Sep 19, 2024
…dule (pythonGH-27394)

Automerge-Triggered-By: GH:benjaminp
(cherry picked from commit 196998e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants