-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Conversation
vstinner
commented
Sep 11, 2024
•
edited
Loading
edited
- Issue: The configure script incorrectly detects crypt() when it doesn't exist #123917
9b0c82c
to
7719552
Compare
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
7719552
to
08b34af
Compare
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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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]>
08b34af
to
522f7cb
Compare