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.

Sign up for GitHub

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

Jump to bottom

Regression in OpenSSL 3.2.0 when compiled without atomics #23376

Closed
link2xt opened this issue Jan 23, 2024 · 2 comments
Closed

Regression in OpenSSL 3.2.0 when compiled without atomics #23376

link2xt opened this issue Jan 23, 2024 · 2 comments
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 help wanted triaged: bug The issue/pr is/fixes a bug

Comments

@link2xt
Copy link

link2xt commented Jan 23, 2024

OpenSSL 3.2.0 fails on a minimal example running SSL_CTX_new(TLS_method()) with an error:

8C9670000000000:error:0A080014:SSL routines:SSL_CTX_new_ex:reason(20):ssl/ssl_lib.c:3929:

I have published the code with instructions on reproducing the problem at https://github.com/link2xt/openssl-zig-regression

I have bisected the problem to commit
fc570b2
merged from PR #20927.

Zig toolchains are commonly used for cross-compilation of C code, see https://andrewkelley.me/post/zig-cc-powerful-drop-in-replacement-gcc-clang.html and https://actually.fyi/posts/zig-makes-rust-cross-compilation-just-work/, https://0110.be/posts/Crossplatform_JNI_builds_with_Zig for examples.

We have successfully used Zig toolchains for cross-compilation of Rust code with C dependencies (including SQLite3 and OpenSSL) at https://github.com/deltachat/deltachat-core-rust/, but with update to OpenSSL 3.2.0 the builds are broken.

Zig toolchains use their own "compiler runtime" which does not have complete atomics support, so we have to compile it with -DBROKEN_CLANG_ATOMICS: deltachat/deltachat-core-rust#4799

Apparently change #20927 was not tested for the case when atomics are not available.
@link2xt link2xt added the issue: bug report The issue was opened to report a bug label Jan 23, 2024
@link2xt link2xt changed the title Regression in OpenSSL 3.2.0 when compiled using Zig 0.11.0 toolchain for x86_64-linux-musl` target Regression in OpenSSL 3.2.0 when compiled using Zig 0.11.0 toolchain for x86_64-linux-musl target Jan 23, 2024
This was referenced Jan 23, 2024
Closed
Closed
@t8m t8m added branch: master Merge to master branch help wanted triaged: bug The issue/pr is/fixes a bug branch: 3.2 Merge to openssl-3.2 and removed issue: bug report The issue was opened to report a bug labels Jan 23, 2024
@kroeckx
Copy link
Member

kroeckx commented Jan 25, 2024

With that define, it falls back to taking a lock. I currently can't see how that should breaks. Can you reproduce this problem with gcc or clang, making sure atomis are not used?

Zig should really implement proper atomics. There are plans to have atomic support mandatory.

@link2xt
Copy link
Author

link2xt commented Jan 27, 2024

I have updated example repo to remove Zig dependency.

Running scripts/init.sh && scripts/build-openssl.sh && scripts/build-example.sh && ./a.out fails with:

4077F64BD4700000:error:0A080014:SSL routines:SSL_CTX_new_ex:reason(20):ssl/ssl_lib.c:3929:

cc --version output:

cc (GCC) 13.2.1 20230801
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@link2xt link2xt changed the title Regression in OpenSSL 3.2.0 when compiled using Zig 0.11.0 toolchain for x86_64-linux-musl target Regression in OpenSSL 3.2.0 when compiled without atomics Jan 27, 2024
@link2xt link2xt mentioned this issue Feb 9, 2024
Closed
@weihanglo weihanglo mentioned this issue Mar 6, 2024
Open
dmage added a commit to dmage/openssl that referenced this issue Apr 10, 2024
@dmage
crypto/provider_core.c: Allocate activatecnt_lock
3b08962 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
  if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
    - It works without the lock, but in general the need for the lock
      depends on __atomic_is_lock_free results
  elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
    - The lock is not needed (unless ret is NULL, which should never happen?)
  else
    - The lock is required
else
  - The lock is not needed
endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes openssl#23376.

Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>
dmage added a commit to dmage/openssl that referenced this issue Apr 10, 2024
@dmage
crypto/provider_core.c: Allocate activatecnt_lock
25f248f 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes openssl#23376.

Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>
dmage added a commit to dmage/openssl that referenced this issue Apr 10, 2024
@dmage
crypto/provider_core.c: Allocate activatecnt_lock
19b3ef4 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
      endif
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes openssl#23376.

Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>
@dmage dmage mentioned this issue Apr 10, 2024
Closed
2 tasks
dmage added a commit to dmage/openssl that referenced this issue Apr 10, 2024
@dmage
crypto/provider_core.c: Allocate activatecnt_lock
27035b9 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
      endif
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes openssl#23376.

CLA: trivial
Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>
openssl-machine pushed a commit that referenced this issue Apr 11, 2024
@dmage @t8m
crypto/provider_core.c: Allocate activatecnt_lock
e661fbc 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
      endif
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes #23376.

CLA: trivial
Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24081)

(cherry picked from commit 2fd6c12)
openssl-machine pushed a commit that referenced this issue Apr 11, 2024
@dmage @t8m
crypto/provider_core.c: Allocate activatecnt_lock
d3de76d 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
      endif
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes #23376.

CLA: trivial
Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24081)

(cherry picked from commit 2fd6c12)
jvdsn pushed a commit to jvdsn/openssl that referenced this issue Jun 3, 2024
@dmage @jvdsn
crypto/provider_core.c: Allocate activatecnt_lock
c123051 
CRYPTO_atomic_add has a lock as a parameter, which is often ignored, but in
some cases (for example, when BROKEN_CLANG_ATOMICS is defined) it is required.

There is no easy way to determine if the lock is needed or not. The current
logic looks like this:

    if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG) && !defined(OPENSSL_SYS_WINDOWS)
      if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) && !defined(BROKEN_CLANG_ATOMICS)
        - It works without the lock, but in general the need for the
          lock depends on __atomic_is_lock_free results
      elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11))
        - The lock is not needed (unless ret is NULL, which should never
          happen?)
      else
        - The lock is required
      endif
    else
      - The lock is not needed
    endif

Adding such conditions outside of crypto.h is error-prone, so it is better to
always allocate the lock, otherwise CRYPTO_atomic_add may silently fail.

Fixes openssl#23376.

CLA: trivial
Fixes: fc570b2 ("Avoid taking a write lock in ossl_provider_doall_activated()")
Signed-off-by: Oleg Bulatov <oleg@bulatov.me>

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24081)
@tuddman tuddman mentioned this issue Jul 29, 2024
Open
@Angel-Dijoux Angel-Dijoux mentioned this issue Nov 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 help wanted triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

crypto/provider_core.c: Allocate activatecnt_lock dmage/openssl
3 participants
@kroeckx @t8m @link2xt