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

crypto/provider_core.c: Allocate activatecnt_lock #24081

Closed
wants to merge 1 commit into from
Closed

crypto/provider_core.c: Allocate activatecnt_lock #24081

wants to merge 1 commit into from

Conversation

dmage
Copy link
Contributor

@dmage dmage commented Apr 10, 2024

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.

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label 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>
@dmage dmage force-pushed the fix-for-broken-clang-atomics branch from 19b3ef4 to 27035b9 Compare April 10, 2024 06:03
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Apr 10, 2024
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug cla: trivial One of the commits is marked as 'CLA: trivial' severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 labels Apr 10, 2024
t8m
t8m approved these changes Apr 10, 2024
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK with CLA: trivial

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Apr 10, 2024
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 10, 2024
@t-j-h
Copy link
Member

t-j-h commented Apr 10, 2024

OK with CLA: trivial

@t-j-h t-j-h added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 10, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 11, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Apr 11, 2024

Merged to the master, 3.3 and 3.2 branches. Thank you for your contribution.

@t8m t8m closed this Apr 11, 2024
openssl-machine pushed a commit that referenced this pull request 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 pull request 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)
openssl-machine pushed a commit that referenced this pull request Apr 11, 2024
@dmage @t8m
crypto/provider_core.c: Allocate activatecnt_lock
2fd6c12 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@t-j-h t-j-h t-j-h approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression in OpenSSL 3.2.0 when compiled without atomics
4 participants
@dmage @t-j-h @openssl-machine @t8m