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

[3.12] gh-123917: Fix crypt check in configure #123952

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 11, 2024

@vstinner vstinner changed the title gh-123917: Fix crypt check in configure [3.12] gh-123917: Fix crypt check in configure Sep 11, 2024
configure.ac Outdated
@@ -5239,9 +5239,9 @@ WITH_SAVE_ENV([
#include <unistd.h>
], [
#ifdef HAVE_CRYPT_R
void *x = crypt_r;
volatile void *x = crypt_r;

Choose a reason for hiding this comment

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

Unfortunately this is not enough. Making an auto variable volatile doesn't prevent the optimizer from removing it. I tried this change first so I know it doesn't work :). That's why in my suggested change, I made x a global volatile variable instead.

Choose a reason for hiding this comment

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

Just to note you can easily test this by putting this code into a sample .c file and then compile/link it without linking -lcrypt and with -O2; you will not get a linker error even though you should. That's the problem.

I am using the latest GCC 14.2 so maybe earlier versions don't remove this? Not sure but easy to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. I modified my PR to avoid volatile, but use the variable value instead.

Please check the updated PR.

configure.ac Outdated
@@ -5239,9 +5239,9 @@ WITH_SAVE_ENV([
#include <unistd.h>
], [
#ifdef HAVE_CRYPT_R
void *x = crypt_r;
volatile void *x = crypt_r;

Choose a reason for hiding this comment

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

Just to note you can easily test this by putting this code into a sample .c file and then compile/link it without linking -lcrypt and with -O2; you will not get a linker error even though you should. That's the problem.

I am using the latest GCC 14.2 so maybe earlier versions don't remove this? Not sure but easy to test.

configure.ac Outdated
@@ -5243,6 +5243,7 @@ WITH_SAVE_ENV([
#else
void *x = crypt;
#endif
return (x != NULL);

Choose a reason for hiding this comment

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

Unfortunately, this also isn't enough to trigger the problematic behavior. I extracted the conftest.c that configure generates so I can test it:

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

int
 main (void)
 {
        #ifdef HAVE_CRYPT_R
          void *x = crypt_r;
        #else
          void *x = crypt;
        #endif
          return (x != NULL);
 }

compiling this without -lcrypt should give an error since the symbol crypt is not defined. However, it does not give an error:

$ gcc -o /tmp/crypt -O2 /tmp/crypt.c
$

If I use the version I suggested, I do get an error:

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

volatile void *x;

int
 main (void)
 {
        #ifdef HAVE_CRYPT_R
          x = crypt_r;
        #else
          x = crypt;
        #endif
          return 0;
 }

then I see:

$ gcc -o /tmp/crypt -O2 /tmp/crypt.c
/bin/ld: /tmp/ccwu2AcU.o: in function `main':
crypt.c:(.text.startup+0x7): undefined reference to `crypt'
collect2: error: ld returned 1 exit status
$

Can you reproduce this behavior in a simple test on your system?

Copy link
Contributor

Choose a reason for hiding this comment

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

Paul's suggested variant looks right to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

compiling this without -lcrypt should give an error since the symbol crypt is not defined. However, it does not give an error:

What's your operating system and GCC version? On Fedora 40, I get an "undefined reference" linker error using GCC 14.2.1 and clang 18.1.6.

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified again the PR to use a global volatile void *func; variable and I kept the != NULL test (to use the variable value). Does it work for you?

Choose a reason for hiding this comment

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

I am using GCC 14.2.0 (I don't know of any actual 14.2.1 release... I wonder where that .1 came from?) / binutils 2.43.1 using GNU gold as the linker, on Red Hat 8.6. I also see the same behavior with GCC 9.4 / binutils 2.34 with GNU ld, on an Ubuntu 20.04 system. I'm very surprised we get such different behavior...! Just for sanity, you're sure you used -O2? As in gcc -O2 -o crypttest crypttest.c? That doesn't give an error for me with the first example code in my comment above. It also succeeds (incorrectly) if I use a volatile auto variable.

Your current latest version with the volatile global variable, does give a link error as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your current latest version with the volatile global variable, does give a link error as expected.

Ok, thanks for testing. I merged my PR.


Building the following code with gcc -O2 x.c -o x works whereas it should fail. I think that in my previous tests, I added -lcrypt to my command line.

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

int
main (void)
{
#ifdef HAVE_CRYPT_R
    void *x = crypt_r;
#else
    void *x = crypt;
#endif
    return (x != NULL);
}

If confirm that building the following code fails as expected with gcc -O2 x.c -o x:

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

volatile void *x;

int
main (void)
{
#ifdef HAVE_CRYPT_R
    x = crypt_r;
#else
    x = crypt;
#endif
    return (x != NULL);
}

Choose a reason for hiding this comment

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

I think that in my previous tests, I added -lcrypt to my command line.

Oh yes, that will definitely do it! :)

Thanks for the work Victor!

Use a global volatile variable and check if the function is not NULL
to use the variable. Otherwise, a compiler optimization can remove
the variable making the check useless.

Co-Authored-By: Paul Smith <[email protected]>
@vstinner vstinner merged commit 53af5b2 into python:3.12 Sep 12, 2024
28 checks passed
@vstinner vstinner deleted the configure_crypt branch September 12, 2024 14:21
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.

3 participants