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

warning: macOS truffleruby-head: TS_VERIFY_CTS_set_certs macro redefined in OpenSSL 3.1 #650

Closed
junaruga opened this issue Jul 17, 2023 · 25 comments · Fixed by #653
Closed

Comments

@junaruga
Copy link
Member

junaruga commented Jul 17, 2023

I found one warning in macOS truffleruby-head case at #618 (comment).

In the case, OpenSSL 3.1.1 is used.

https://github.com/ruby/openssl/actions/runs/5580792815/jobs/10198185535#step:8:94

../../../../ext/openssl/openssl_missing.h:195:11: error: 'TS_VERIFY_CTS_set_certs' macro redefined [-Werror,-Wmacro-redefined]
 #  define TS_VERIFY_CTS_set_certs(ctx, crts) ((ctx)->certs=(crts))
          ^
/usr/local/Cellar/openssl@3/3.1.1_1/include/openssl/ts.h:426:11: note: previous definition is here
#  define TS_VERIFY_CTS_set_certs(ctx, cert) TS_VERIFY_CTX_set_certs(ctx,cert)
          ^

In the last success case, the OpenSSL 1.1.1u was used in the macOS truffleruby-head case.
https://github.com/ruby/openssl/actions/runs/5532198527/jobs/10093943683#step:9:17

@junaruga junaruga changed the title macOS truffleruby-head: TS_VERIFY_CTS_set_certs macro redefined in OpenSSL 3.1 warning: macOS truffleruby-head: TS_VERIFY_CTS_set_certs macro redefined in OpenSSL 3.1 Jul 17, 2023
@junaruga
Copy link
Member Author

It seems this part in OpenSSL 3.1.1.

https://github.com/openssl/openssl/blob/openssl-3.1.1/include/openssl/ts.h#L425-L427

# ifndef OPENSSL_NO_DEPRECATED_3_0
#  define TS_VERIFY_CTS_set_certs(ctx, cert) TS_VERIFY_CTX_set_certs(ctx,cert)
# endif

@junaruga junaruga assigned rhenium and unassigned rhenium Jul 17, 2023
@rhenium
Copy link
Member

rhenium commented Jul 18, 2023

From the log:

[...]
checking for SSL_set0_tmp_dh_pkey(NULL, NULL) in openssl/ssl.h... no
checking for ERR_get_error_all(NULL, NULL, NULL, NULL, NULL) in openssl/err.h... no
checking for TS_VERIFY_CTX_set_certs(NULL, NULL) in openssl/ts.h... no
checking for SSL_CTX_load_verify_file(NULL, "") in openssl/ssl.h... no
checking for BN_check_prime(NULL, NULL, NULL) in openssl/bn.h... no
checking for EVP_MD_CTX_get0_md(NULL) in openssl/evp.h... no
checking for EVP_MD_CTX_get_pkey_ctx(NULL) in openssl/evp.h... no
checking for EVP_PKEY_eq(NULL, NULL) in openssl/evp.h... no
checking for EVP_PKEY_dup(NULL) in openssl/evp.h... no
checking for whether -Werror is accepted as CFLAGS... yes
creating extconf.h
creating Makefile
[...]
/Users/runner/.rubies/truffleruby-head/lib/sulong/native/bin/graalvm-native-clang -I. -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I/Users/runner/.rubies/truffleruby-head/lib/cext/include/ruby/backward -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I../../../../ext/openssl -I/usr/local/Cellar/openssl@3/3.1.1_1/include -DRUBY_EXTCONF_H=\"extconf.h\"  -D_DARWIN_C_SOURCE -DTRUFFLERUBY_ABI_VERSION=3.1.3.9 -I/usr/local/opt/openssl@1.1/include -fPIC   -Werror=implicit-function-declaration -Wno-int-conversion -Wno-int-to-pointer-cast -Wno-incompatible-pointer-types -Wno-format-invalid-specifier -Wno-format-extra-args -ferror-limit=500  -Werror  -o openssl_missing.o -c ../../../../ext/openssl/openssl_missing.c 

This doesn't seem correct. Both OpenSSL 1.1.1 and 3.1.1 are included.

@junaruga
Copy link
Member Author

This doesn't seem correct. Both OpenSSL 1.1.1 and 3.1.1 are included.

Hmm. Right. I can see both the -I/usr/local/Cellar/openssl@3/3.1.1_1/include and -I/usr/local/opt/openssl@1.1/include in the mentioned gcc command line in the log.

@junaruga
Copy link
Member Author

Hi @eregon, I guess that you are familiar with the truffleruby things. We see the issue that the graalvm-native-clang compiler includes both OpenSSL 1.1 and 3.1.1 header directories on the -I arguments in GitHub Actions macOS truffleruby-head case. And I assume that the situation makes the compiler warning above. Do you know what is the cause of this issue?

@junaruga
Copy link
Member Author

I opened the ticket for ruby-build: rbenv/ruby-build#2220

@junaruga
Copy link
Member Author

I tested by removing OpenSSL 1.1 include directory in the macos-latest truffleruby-head case. But the compiler warning still happens.

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 36609e2..9134c5b 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -57,6 +57,13 @@ jobs:
         run: echo "OPENSSL_MODULES=$($env:RI_DEVKIT)\$($env:MSYSTEM_PREFIX)\lib\ossl-modules" >> $env:GITHUB_ENV
         if: runner.os == 'Windows' && matrix.ruby == '3.2'
 
+      # A temporary workaround to avoid a compiler warning.
+      # https://github.com/rbenv/ruby-build/issues/2220
+      - name: remove openssl 1.1 include directory
+        run: |
+          rm -rfv "$(brew --prefix openssl@1.1)/include"
+        if: matrix.os == 'macos-latest' && matrix.ruby == 'truffleruby-head'
+
       - name: compile
         run:  rake compile -- --enable-debug

Here is the log.
https://github.com/junaruga/openssl/actions/runs/5615788064/job/15216818664

@junaruga
Copy link
Member Author

Even when skipping the compiler warning check in the CI case, the compiler error happens in my tests.

https://github.com/junaruga/openssl/actions/runs/5621718703/job/15233027951#step:9:101

 ../../../../ext/openssl/ossl_hmac.c:249:35: error: incomplete definition of type 'struct evp_md_ctx_st'
    pkey = EVP_PKEY_CTX_get0_pkey(EVP_MD_CTX_get_pkey_ctx(ctx));
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

However I found the workaround for this issue. It seems that the problem is openssl@3 brew package's include directory is added as the compiler's -I <include directory> argument. I sent the PR #652.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

Hi @eregon, I guess that you are familiar with the truffleruby things. We see the issue that the graalvm-native-clang compiler includes both OpenSSL 1.1 and 3.1.1 header directories on the -I arguments in GitHub Actions macOS truffleruby-head case. And I assume that the situation makes the compiler warning above. Do you know what is the cause of this issue?

I'm confident TruffleRuby doesn't do this. So is this repo/the extconf.rb adding openssl in CPPFLAGS or so?
That's not well supported in general. TruffleRuby was compiled against the system openssl when it's installed. Trying to install a different openssl gem linking to another libssl is unlikely to work.
Notably because when building CRuby/TruffleRuby openssl paths might end up in RbConfig::CONFIG["CPPFLAGS"] etc if --with-openssl-dir was used.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

It's related to this code: https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/lib/truffle/rbconfig.rb#L97-L107
And https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/lib/truffle/truffle/openssl-prefix.rb
As it says, we need C extensions use the same libssl as the one used for the openssl C extension.

Maybe we should prefer libssl 3 to 1.1 in https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/lib/truffle/truffle/openssl-prefix.rb
We didn't so far because using libssl 3 was not fully compatible.

Do you know what picks openssl 3 over 1.1 on macOS so that this repo CI's fail?
TruffleRuby would always pick 1.1, so something else must tell it to use 3.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

I think the problem is that truffleruby-head is built on macos 11 but in that CI run is used on macos 12. And probably one of them has openssl 1.1 and the other not.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

According to
https://github.com/actions/runner-images/blob/main/images/macos/macos-11-Readme.md
https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md
They both have only openssl 1.1.
But maybe those .md files are incorrect.

Because where does openssl 3 come from then?

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Jul 21, 2023

FYI, using https://github.com/MSP-Greg/actions-image-testing/actions, but installing MRI Ruby may install another version?

macOS   'openssl version'
13      LibreSSL 3.3.6
12      OpenSSL 1.1.1u  30 May 2023
11      OpenSSL 1.1.1u  30 May 2023

@eregon
Copy link
Member

eregon commented Jul 21, 2023

With https://github.com/eregon/actions-shell/actions/runs/5623122665/job/15237147385

11:
$ brew list | grep openssl
openssl@1.1
openssl@3
12:
$ brew list | grep openssl
openssl@1.1
openssl@3

So something is quite weird here, the TruffleRuby logic should always consistently pick 1.1.
Does this gem have any logic to locate an openssl in homebrew?

@eregon
Copy link
Member

eregon commented Jul 21, 2023

Also it seemed to work fine on the last CI run on master: https://github.com/ruby/openssl/actions/runs/5532198527/job/14981073682

@rhenium
Copy link
Member

rhenium commented Jul 21, 2023

The difference between master and the PR is the runner image version: 20230623.2 vs 20230709.1. Some packages from Homebrew might have started to depend on openssl@3 in the meantime.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

In this command line:

/Users/runner/.rubies/truffleruby-head/lib/sulong/native/bin/graalvm-native-clang -I. -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I/Users/runner/.rubies/truffleruby-head/lib/cext/include/ruby/backward -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I../../../../ext/openssl -I/usr/local/Cellar/openssl@3/3.1.1_1/include -DRUBY_EXTCONF_H="extconf.h" -D_DARWIN_C_SOURCE -DTRUFFLERUBY_ABI_VERSION=3.1.3.9 -I/usr/local/opt/openssl@1.1/include -fPIC -Werror=implicit-function-declaration -Wno-int-conversion -Wno-int-to-pointer-cast -Wno-incompatible-pointer-types -Wno-format-invalid-specifier -Wno-format-extra-args -ferror-limit=500 -Werror -o openssl_missing.o -c ../../../../ext/openssl/openssl_missing.c

This part is from mkmf or openssl extconf.rb:

/Users/runner/.rubies/truffleruby-head/lib/sulong/native/bin/graalvm-native-clang -I. -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I/Users/runner/.rubies/truffleruby-head/lib/cext/include/ruby/backward -I/Users/runner/.rubies/truffleruby-head/lib/cext/include -I../../../../ext/openssl -I/usr/local/Cellar/openssl@3/3.1.1_1/include -DRUBY_EXTCONF_H="extconf.h"

This part is from TruffleRuby:

-D_DARWIN_C_SOURCE -DTRUFFLERUBY_ABI_VERSION=3.1.3.9 -I/usr/local/opt/openssl@1.1/include -fPIC -Werror=implicit-function-declaration -Wno-int-conversion -Wno-int-to-pointer-cast -Wno-incompatible-pointer-types -Wno-format-invalid-specifier -Wno-format-extra-args -ferror-limit=500

This part is from mkmf or openssl extconf.rb:

-Werror -o openssl_missing.o -c ../../../../ext/openssl/openssl_missing.c

Probably it's

pkg_config_found = !dir_config_given && pkg_config("openssl") && have_header("openssl/ssl.h")

and that somehow changed with the new macos image. I'll check.

@rhenium
Copy link
Member

rhenium commented Jul 21, 2023

It's related to this code: https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/lib/truffle/rbconfig.rb#L97-L107

I wonder if it can use RbConfig::CONFIG["configure_args"] = "'--with-openssl-dir=#{openssl_prefix}'". This is where the flag is stored in CRuby when it is passed to CRuby's configure.

openssl (and (I think) majority of other native extension gems that uses OpenSSL also) use dir_config("openssl") and will pick up the flag.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

https://github.com/eregon/actions-shell/actions/runs/5623553265/job/15238480143

$ pkg-config --modversion openssl
3.1.1

So pkg-config prefers libssl 3 and that's how it's found. And somehow that pkg-config knows about Homebrew.
And the conflict with preferring 1.1 if available.

In the bundled openssl gem in TruffleRuby there is https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/src/main/c/openssl/extconf.rb#L16-L21
So that whatever prefix truffle/openssl-prefix chooses it's respected and we don't rely on pkg-config which is rather unpredictable.

Maybe we can upstream piece of code here, @rhenium WDYT?

@eregon
Copy link
Member

eregon commented Jul 21, 2023

Fix in #653

Regarding RbConfig::CONFIG["configure_args"], yes that would be nice but is rather tricky since there is no really a "configure time" for TruffleRuby or a generated rbconfig.rb. We'd need to save the result of truffle/openssl-prefix in some file, and have rbconfig.rb read from it or so.

FYI the approach taken in TruffleRuby is similar to what happens on CRuby when using --with-opt-dir, like ruby-install does it:
https://github.com/postmodern/ruby-install/blob/f4978056b54f6926b5c46d86aae19a90544bdd60/share/ruby-install/ruby/functions.sh#L39
I filed oracle/truffleruby#3170

@rhenium
Copy link
Member

rhenium commented Jul 21, 2023

Even if there are both OpenSSL 1.1.1 and 3.1 in the paths, I find it a bit odd that mkmf.rb fails to detect TS_VERIFY_CTS_set_certs(NULL, NULL) because it exists in both versions (as a function in 1.1.1, as a macro in 3.x).

Looking at the mkmf.log (https://github.com/rhenium/ruby-openssl/actions/runs/5623513410/job/15238357483), *flags from RbConfig and those from pkg-config are joined in an inconsistent order:

have_func: checking for TS_VERIFY_CTS_set_certs(NULL, NULL) in openssl/ts.h... -------------------- no

LD_LIBRARY_PATH=. "/Users/runner/.rubies/truffleruby-head/lib/sulong/native/bin/graalvm-native-clang
	-o conftest
	-I/Users/runner/.rubies/truffleruby-head/lib/cext/include
	-I/Users/runner/.rubies/truffleruby-head/lib/cext/include/ruby/backward
	-I/Users/runner/.rubies/truffleruby-head/lib/cext/include
	-I../../../../ext/openssl
	-I/usr/local/Cellar/openssl@3/3.1.1_1/include   <================== $INCFLAGS (from pkg-config)
	-D_DARWIN_C_SOURCE
	-DTRUFFLERUBY_ABI_VERSION=3.1.3.9 
	-I/usr/local/opt/openssl@1.1/include            <================== $CFLAGS (from RbConfig::CONFIG["cflags"])
	-Werror=implicit-function-declaration
	-Wno-int-conversion
	-Wno-int-to-pointer-cast
	-Wno-incompatible-pointer-types
	-Wno-format-invalid-specifier
	-Wno-format-extra-args
	-ferror-limit=500  conftest.c 
	-L. 
	-L/usr/local/opt/openssl@1.1/lib                <================== $LDFLAGS (from RbConfig::CONFIG["ldflags"])
	-L/usr/local/Cellar/openssl@3/3.1.1_1/lib       <================== $LDFLAGS (from pkg-config)
	-lssl
	-lcrypto
	-lgraalvm-llvm 
	-lssl
	-lcrypto  
	-L/Users/runner/.rubies/truffleruby-head/lib/cext
	-rpath /Users/runner/.rubies/truffleruby-head/lib/cext
	-ltruffleruby
	-rpath /Users/runner/.rubies/truffleruby-head/lib/sulong/native/lib"

This seems to be a change in mkmf.rb in ruby/ruby@097c3e9 (Ruby 2.2), which treats -I flags and others (nothing in this case) from pkg-config separately.

I don't know what was the motivation behind the change as it doesn't have a link to the issue tracker, or if this situation should even be supported.

@eregon
Copy link
Member

eregon commented Jul 21, 2023

Indeed, that sounds quite problematic for any usage of pkg-config, could you file an issue at https://bugs.ruby-lang.org/ about this?
Notably if it accidentally works with different version of headers and libs for some native library.

@rhenium
Copy link
Member

rhenium commented Jul 21, 2023

I've filed https://bugs.ruby-lang.org/issues/19778 for mkmf's pkg_config.

@rhenium
Copy link
Member

rhenium commented Jul 21, 2023

Regarding RbConfig::CONFIG["configure_args"], yes that would be nice but is rather tricky since there is no really a "configure time" for TruffleRuby or a generated rbconfig.rb. We'd need to save the result of truffle/openssl-prefix in some file, and have rbconfig.rb read from it or so.

I don't see a technical reason it can't just set RbConfig::CONFIG["configure_args"] since it's currently able to modify RbConfig::CONFIG["*FLAGS"].

I think this is a good idea. Having --with-openssl-dir flag prevents pkg_config() from being called at all in openssl gem. Other gems might still call it, but mkmf.rb included in Ruby >= 3.1 (which I assume TruffleRuby copies) should use a PKG_CONFIG_PATH environment variable to point the directory passed to --with-openssl-dir, so pkg-config will just return the same set of flags.

@eregon
Copy link
Member

eregon commented Jul 24, 2023

I don't see a technical reason it can't just set RbConfig::CONFIG["configure_args"] since it's currently able to modify RbConfig::CONFIG["*FLAGS"].

Ah you're right, we could set it on macOS like we do for *FLAGS, and then we wouldn't need https://github.com/oracle/truffleruby/blob/16db2e64472e5f4d88176ae122815b6b291c0947/src/main/c/openssl/extconf.rb#L16-L21
I think at the time I didn't know dir_config looks in configure_args or didn't realize this way.

graalvmbot pushed a commit to oracle/truffleruby that referenced this issue Jul 24, 2023
@eregon
Copy link
Member

eregon commented Jul 24, 2023

I'm trying that approach in oracle/truffleruby#3174, if it works and when it's merged we can revert #653.

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

Successfully merging a pull request may close this issue.

4 participants