Skip to content

bpo-28503: _crypt: fix implicit declaration of crypt(), use crypt_r() if available. #4691

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

Closed
wants to merge 1 commit into from

Conversation

EdSchouten
Copy link

@EdSchouten EdSchouten commented Dec 3, 2017

The '_crypt' module provides a binding to the C crypt(3) function. It is
used by the crypt.crypt() function.

Looking at the C code, there are a couple of things we can improve:

  • Because crypt() only depends on primitive C types, we currently get
    away with calling it without it being declared. Ensure that we include
    <unistd.h>, which is the POSIX header file declaring this.

  • The disadvantage of crypt() is that it's thread-unsafe. Systems like
    Linux and recent versions of FreeBSD nowadays provide crypt_r() as a
    replacement. This function allows you to pass in a 'crypt_data' object
    that will hold the resulting string for you. Extend the code to use
    this function when available.

This patch is actually needed to make this module build on CloudABI
(https://mail.python.org/pipermail/python-dev/2016-July/145708.html).
CloudABI's C library doesn't provide any thread-unsafe functions,
meaning that crypt_r() is the only way you can crypt passwords.

https://bugs.python.org/issue28503

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@EdSchouten EdSchouten changed the title _crypt: fix implicit declaration of crypt(), use crypt_r() if available. bpo-28503: _crypt: fix implicit declaration of crypt(), use crypt_r() if available. Dec 3, 2017
The '_crypt' module provides a binding to the C crypt(3) function. It is
used by the crypt.crypt() function.

Looking at the C code, there are a couple of things we can improve:

- Because crypt() only depends on primitive C types, we currently get
  away with calling it without it being declared. Ensure that we include
  <unistd.h>, which is the POSIX header file declaring this.

- The disadvantage of crypt() is that it's thread-unsafe. Systems like
  Linux and recent versions of FreeBSD nowadays provide crypt_r() as a
  replacement. This function allows you to pass in a 'crypt_data' object
  that will hold the resulting string for you. Extend the code to use
  this function when available.

This patch is actually needed to make this module build on CloudABI
(https://mail.python.org/pipermail/python-dev/2016-July/145708.html).
CloudABI's C library doesn't provide any thread-unsafe functions,
meaning that crypt_r() is the only way you can crypt passwords.
@@ -2679,6 +2679,7 @@ AC_MSG_RESULT($SHLIBS)
AC_CHECK_LIB(sendfile, sendfile)
AC_CHECK_LIB(dl, dlopen) # Dynamic linking for SunOS/Solaris and SYSV
AC_CHECK_LIB(dld, shl_load) # Dynamic linking for HP-UX
AC_CHECK_LIB(crypt, crypt) # crypt() on Linux
Copy link
Member

Choose a reason for hiding this comment

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

HAVE_LIBCRYPT created by this doesn't seem to be used in Modules/_cryptmodule.c. I recently tried to use AC_CHECK_LIB for the same use case, but there were some comments concerning the test program created by autotools. I end up using the following approach:

AC_MSG_CHECKING(for uuid_generate_time_safe)
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <uuid/uuid.h>]], [[
#ifndef uuid_generate_time_safe
void *x = uuid_generate_time_safe
#endif
]])],
  [AC_DEFINE(HAVE_UUID_GENERATE_TIME_SAFE, 1, Define if uuid_generate_time_safe() exists.)
   AC_MSG_RESULT(yes)],
  [AC_MSG_RESULT(no)]
)

@dimpase
Copy link
Contributor

dimpase commented Jun 20, 2018

Any progress on this? It appears to be needed for the latest Fedora, where they switched to gcc 8.1.

@jdemeyer
Copy link
Contributor

Because crypt() only depends on primitive C types, we currently get
away with calling it without it being declared.

Are you sure? Undeclared functions are assumed a return type of int while crypt() returns char *. I'm guessing that this might cause the Fedora 28 problem, which uses a different crypt library https://github.com/besser82/libxcrypt and does not declare crypt() in <unistd.h>

This PR does fix that problem.

@dimpase
Copy link
Contributor

dimpase commented Jun 22, 2018

Moreover, it's getting urgent, as there are plans to remove crypt() from glibc. Then calls to crypt without a prototype are doomed. You might convince yourself that it is so by trying to compile and run the following

/* cry.c */
#include <stdio.h>
#ifdef CRYP
#include <crypt.h>
#endif
int main()
{
char *word="blah";
char *salt="foo";
char *res;
res = crypt(word, salt);
printf("%s\n",res);
return 0;
}

Building (on a 64-bit Linux) with -DCRYP -lcrypt gives working program, building just with -lcrypt gives a program that segfaults.


Perhaps it makes sense to split this PR into two - one making crypt() properly tested for, the other adding crypt_r() use, if available.

@jdemeyer
Copy link
Contributor

Linking with libxcrypt is already fixed in https://bugs.python.org/issue32635

@gpshead
Copy link
Member

gpshead commented Dec 30, 2018

#11373 is a modern version of this. I'm waiting on the CI over there before merging it, but rather than anyone spending time updating and addressing conflicts in this one I'm just going to close it.

Our overall concepts are similar. I did the configure.ac check a bit differently to account for #define _GNU_SOURCE being required on Linux. The crypt.h was moved to Python.h a while back independent this PR so mine reuses that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants