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

Check for OpenSSL functions in headers #520

Merged
merged 2 commits into from
Jul 25, 2022
Merged

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Jun 27, 2022

While building with a custom build of OpenSSL, I noticed in mkmf.log
that all the feature detection checks are done using a program lacking
an OpenSSL header include. mkmf retries using a fallback program when
this fails, but that means all the have_func calls compile twice when
compiling once should suffice. Example log without this commit:

have_func: checking for X509_STORE_CTX_get0_cert()... -------------------- yes

DYLD_FALLBACK_LIBRARY_PATH=.:../.. "clang -o conftest ...
conftest.c:14:57: error: use of undeclared identifier 'X509_STORE_CTX_get0_cert'
int t(void) { void ((*volatile p)()); p = (void ((*)()))X509_STORE_CTX_get0_cert; return !p; }
                                                        ^
1 error generated.
checked program was:
/* begin */
 1: #include "ruby.h"
 2:
 3: /*top*/
 4: extern int t(void);
 5: int main(int argc, char **argv)
 6: {
 7:   if (argc > 1000000) {
 8:     int (* volatile tp)(void)=(int (*)(void))&t;
 9:     printf("%d", (*tp)());
10:   }
11:
12:   return !!argv[argc];
13: }
14: int t(void) { void ((*volatile p)()); p = (void ((*)()))X509_STORE_CTX_get0_cert; return !p; }
/* end */

DYLD_FALLBACK_LIBRARY_PATH=.:../.. "clang -o conftest ...
checked program was:
/* begin */
 1: #include "ruby.h"
 2:
 3: /*top*/
 4: extern int t(void);
 5: int main(int argc, char **argv)
 6: {
 7:   if (argc > 1000000) {
 8:     int (* volatile tp)(void)=(int (*)(void))&t;
 9:     printf("%d", (*tp)());
10:   }
11:
12:   return !!argv[argc];
13: }
14: extern void X509_STORE_CTX_get0_cert();
15: int t(void) { X509_STORE_CTX_get0_cert(); return 0; }
/* end */

The second compilation succeeds.

Specify the header for each checked function.

While building with a custom build of OpenSSL, I noticed in mkmf.log
that all the feature detection checks are done using a program lacking
an OpenSSL header include. `mkmf` retries using a fallback program when
this fails, but that means all the `have_func` calls compile twice when
compiling once should suffice. Example log without this commit:

    have_func: checking for X509_STORE_CTX_get0_cert()... -------------------- yes

    DYLD_FALLBACK_LIBRARY_PATH=.:../.. "clang -o conftest ...
    conftest.c:14:57: error: use of undeclared identifier 'X509_STORE_CTX_get0_cert'
    int t(void) { void ((*volatile p)()); p = (void ((*)()))X509_STORE_CTX_get0_cert; return !p; }
                                                            ^
    1 error generated.
    checked program was:
    /* begin */
     1: #include "ruby.h"
     2:
     3: /*top*/
     4: extern int t(void);
     5: int main(int argc, char **argv)
     6: {
     7:   if (argc > 1000000) {
     8:     int (* volatile tp)(void)=(int (*)(void))&t;
     9:     printf("%d", (*tp)());
    10:   }
    11:
    12:   return !!argv[argc];
    13: }
    14: int t(void) { void ((*volatile p)()); p = (void ((*)()))X509_STORE_CTX_get0_cert; return !p; }
    /* end */

    DYLD_FALLBACK_LIBRARY_PATH=.:../.. "clang -o conftest ...
    checked program was:
    /* begin */
     1: #include "ruby.h"
     2:
     3: /*top*/
     4: extern int t(void);
     5: int main(int argc, char **argv)
     6: {
     7:   if (argc > 1000000) {
     8:     int (* volatile tp)(void)=(int (*)(void))&t;
     9:     printf("%d", (*tp)());
    10:   }
    11:
    12:   return !!argv[argc];
    13: }
    14: extern void X509_STORE_CTX_get0_cert();
    15: int t(void) { X509_STORE_CTX_get0_cert(); return 0; }
    /* end */

The second compilation succeeds.

Specify the header for each checked function.
@XrXr
Copy link
Member Author

XrXr commented Jun 27, 2022

This change revealed that X509_STORE_get_ex_new_index is a macro, so have_func() should never be able to find it.

conftest.c:16:41: error: too few arguments provided to function-like macro invocation
extern void X509_STORE_get_ex_new_index();
                                        ^
/Users/a/openssl-1.1.1p/prefix/include/openssl/x509_vfy.h:330:9: note: macro 'X509_STORE_get_ex_new_index' defined here
#define X509_STORE_get_ex_new_index(l, p, newf, dupf, freef) \
        ^
conftest.c:17:43: error: too few arguments provided to function-like macro invocation
int t(void) { X509_STORE_get_ex_new_index(); return 0; }
                                          ^
/Users/a/openssl-1.1.1p/prefix/include/openssl/x509_vfy.h:330:9: note: macro 'X509_STORE_get_ex_new_index' defined here
#define X509_STORE_get_ex_new_index(l, p, newf, dupf, freef) \
        ^

From https://www.openssl.org/docs/man1.1.1/man3/X509_STORE_get_ex_new_index.html:

TYPE_get_ex_new_index() is a macro that calls CRYPTO_get_ex_new_index() with the correct index value.

@nobu
Copy link
Member

nobu commented Jul 9, 2022

https://github.com/nobu/openssl/runs/7264339285?check_suite_focus=true#step:5:56
You can pass matching arguments to pass a macro too.

@nobu
Copy link
Member

nobu commented Jul 9, 2022

But not so significant speed gain as I expected, unfortunately. 😢

X509_STORE_get_ex_new_index() is a macro, so passing just its name to
have_func() doesn't detect it. Pass an example call instead.

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
@rhenium
Copy link
Member

rhenium commented Jul 25, 2022

Looks good to me. Thanks!

@rhenium rhenium merged commit 8752d9e into ruby:master Jul 25, 2022
@rhenium
Copy link
Member

rhenium commented Jul 25, 2022

Now that LibreSSL supports most of the OpenSSL 1.1 functions, I wonder if we can simply replace all those # added in 1.1.0 entries with OPENSSL_VERSION_NUMBER check. OpenSSL 1.0.2 is the only version that needs it.

It could further cut extconf.rb in half.

@XrXr XrXr deleted the check-in-header branch July 25, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants