read lock store on ossl_method_store_do_all by nhorman · Pull Request #24782 · openssl/openssl

nhorman · 2024-07-02T19:14:57Z

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating.

However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock.

The choices to fix it are I think:

add an argument to indicate to ossl_method_store_do_all weather to
take the read or write lock when doing iterations, and add an
is_locked api to the ossl_property_[read|write] lock family so that
subsequent callers can determine if they need to take a lock or not
Clone the algs sparse array in ossl_method_store_do_all and use the
clone to iterate with no lock held, ensuring that updates to the
parent copy of the sparse array are left untoucheTheres a data race
between ossl_method_store_insert and ossl_method_store_do_all, as the
latter doesn't take the property lock before iterating.

I think method (2), while being a bit more expensive, is probably the far less invasive way to go here

Fixes #24672

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untouched during the iteration I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672

paulidale

Just one nit.

This is the method we use elsewhere to avoid similar problems.

paulidale · 2024-07-02T22:38:42Z

A thought: would using a stack for the temporary copy make sense? It would avoid some of the inefficiencies of the sparse array.

t8m · 2024-07-03T07:05:04Z

A thought: would using a stack for the temporary copy make sense? It would avoid some of the inefficiencies of the sparse array.

Yeah, we should use that.

t8m · 2024-07-03T07:05:47Z

Please also note there is a duplicated text in the commit message.

read lock store on ossl_method_store_do_all Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672

nhorman · 2024-07-03T13:39:02Z

fixups made to use a stack rather than a sparse array, fix nits, and correct commit message

t8m

One minor nit.

This is a combination of 3 commits. This is the 1st commit message: read lock store on ossl_method_store_do_all Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untouched during the iteration I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672 This is the commit message #2: amend! read lock store on ossl_method_store_do_all read lock store on ossl_method_store_do_all Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672 This is the commit message #3: fixup! amend! read lock store on ossl_method_store_do_all

openssl-machine · 2024-07-09T09:00:17Z

This pull request is ready to merge

t8m · 2024-07-09T09:28:58Z

Merged to all the active branches. Thank you.

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #24782) (cherry picked from commit d8def79)

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #24782)

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #24782) (cherry picked from commit d8def79)

This reverts commit ee2bb85. The PR was not finished yet, more commits were added later on.

read lock store on ossl_method_store_do_all Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#24782) (cherry picked from commit d8def79)

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#24782) (cherry picked from commit d8def79838cd0d5e7c21d217aa26edb5229f0ab4) Signed-off-by: 王静 <[email protected]>

read lock store on ossl_method_store_do_all Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes openssl#24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#24782) (cherry picked from commit d8def79)

Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#24782) (cherry picked from commit d8def79838cd0d5e7c21d217aa26edb5229f0ab4) Signed-off-by: 王静 <[email protected]>

PR openssl#24782 introduced a copying of the algs stack in ossl_method_store_do all, so that the subsequent iteration of elements through alg_do_one could be done without a lock, and without triggering a tsan error as reported in: openssl#24672 However, the problem wasn't completely fixed. Issue: openssl#27726 Noted that, sometimes we still get a crash when iterating over each algs impls stack. This occurs because, even though we've cloned the algs to a private data area, the impls stack for each alg still points to shared data, which we are accessing without the benefit of a lock. Because of that, if some other thread calls a function that mutates the impl stack (say ossl_method_store_remove()), we may encounter a NULL or garbage value in one of the impl stack values, leading to an unexpected NULL pointer or simmilar, which in turn leads to a crash. Unfortunately we can't use a lock to create exclusive access here, as there are several paths that lead to a recursive mutation of the stack, which would deadlock. So the only way that I see to prevent this (which is admittedly ugly), is to not only clone the alg stack, but to duplicate each algs impl stack with the read lock held, prior to doing the iteration. Further, we've been unable to test this, as the problem is rare, and we don't have a solid reproducer for the issue, but visual inspection suggests this should fix that. Hopefully: Fixes openssl#27726

PR #24782 introduced a copying of the algs stack in ossl_method_store_do all, so that the subsequent iteration of elements through alg_do_one could be done without a lock, and without triggering a tsan error as reported in: #24672 However, the problem wasn't completely fixed. Issue: #27726 Noted that, sometimes we still get a crash when iterating over each algs impls stack. This occurs because, even though we've cloned the algs to a private data area, the impls stack for each alg still points to shared data, which we are accessing without the benefit of a lock. Because of that, if some other thread calls a function that mutates the impl stack (say ossl_method_store_remove()), we may encounter a NULL or garbage value in one of the impl stack values, leading to an unexpected NULL pointer or simmilar, which in turn leads to a crash. Unfortunately we can't use a lock to create exclusive access here, as there are several paths that lead to a recursive mutation of the stack, which would deadlock. So the only way that I see to prevent this (which is admittedly ugly), is to not only clone the alg stack, but to duplicate each algs impl stack with the read lock held, prior to doing the iteration. Further, we've been unable to test this, as the problem is rare, and we don't have a solid reproducer for the issue, but visual inspection suggests this should fix that. Hopefully: Fixes #27726 Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28783) (cherry picked from commit 9ef4f42)

PR #24782 introduced a copying of the algs stack in ossl_method_store_do all, so that the subsequent iteration of elements through alg_do_one could be done without a lock, and without triggering a tsan error as reported in: #24672 However, the problem wasn't completely fixed. Issue: #27726 Noted that, sometimes we still get a crash when iterating over each algs impls stack. This occurs because, even though we've cloned the algs to a private data area, the impls stack for each alg still points to shared data, which we are accessing without the benefit of a lock. Because of that, if some other thread calls a function that mutates the impl stack (say ossl_method_store_remove()), we may encounter a NULL or garbage value in one of the impl stack values, leading to an unexpected NULL pointer or simmilar, which in turn leads to a crash. Unfortunately we can't use a lock to create exclusive access here, as there are several paths that lead to a recursive mutation of the stack, which would deadlock. So the only way that I see to prevent this (which is admittedly ugly), is to not only clone the alg stack, but to duplicate each algs impl stack with the read lock held, prior to doing the iteration. Further, we've been unable to test this, as the problem is rare, and we don't have a solid reproducer for the issue, but visual inspection suggests this should fix that. Hopefully: Fixes #27726 Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28783)

PR #24782 introduced a copying of the algs stack in ossl_method_store_do all, so that the subsequent iteration of elements through alg_do_one could be done without a lock, and without triggering a tsan error as reported in: #24672 However, the problem wasn't completely fixed. Issue: #27726 Noted that, sometimes we still get a crash when iterating over each algs impls stack. This occurs because, even though we've cloned the algs to a private data area, the impls stack for each alg still points to shared data, which we are accessing without the benefit of a lock. Because of that, if some other thread calls a function that mutates the impl stack (say ossl_method_store_remove()), we may encounter a NULL or garbage value in one of the impl stack values, leading to an unexpected NULL pointer or simmilar, which in turn leads to a crash. Unfortunately we can't use a lock to create exclusive access here, as there are several paths that lead to a recursive mutation of the stack, which would deadlock. So the only way that I see to prevent this (which is admittedly ugly), is to not only clone the alg stack, but to duplicate each algs impl stack with the read lock held, prior to doing the iteration. Further, we've been unable to test this, as the problem is rare, and we don't have a solid reproducer for the issue, but visual inspection suggests this should fix that. Hopefully: Fixes #27726 Reviewed-by: Saša Nedvědický <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28783) (cherry picked from commit 9ef4f42)

nhorman self-assigned this Jul 2, 2024

nhorman marked this pull request as draft July 2, 2024 19:15

nhorman mentioned this pull request Jul 2, 2024

Tsan data race between sa_doall and ossl_sa_set #24672

Closed

github-actions Bot added the severity: fips change The pull request changes FIPS provider sources label Jul 2, 2024

nhorman marked this pull request as ready for review July 2, 2024 19:36

paulidale approved these changes Jul 2, 2024

View reviewed changes

Comment thread crypto/property/property.c Outdated

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending labels Jul 3, 2024

t8m requested changes Jul 3, 2024

View reviewed changes

Comment thread crypto/property/property.c Outdated

fixup! amend! read lock store on ossl_method_store_do_all

d978ba3

nhorman requested a review from t8m July 3, 2024 15:15

t8m approved these changes Jul 3, 2024

View reviewed changes

t8m removed the approval: otc review pending label Jul 3, 2024

paulidale reviewed Jul 4, 2024

View reviewed changes

Comment thread crypto/property/property.c Outdated

paulidale self-requested a review July 4, 2024 03:26

fixup! fixup! amend! read lock store on ossl_method_store_do_all

b68186f

t8m approved these changes Jul 8, 2024

View reviewed changes

openssl-machine removed the approval: done This pull request has the required number of approvals label Jul 9, 2024

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jul 9, 2024

t8m closed this Jul 9, 2024

rschu1ze added a commit to ClickHouse/openssl that referenced this pull request Jul 14, 2024

Revert "OpenSSL fix openssl#24782"

ffc34c7

This reverts commit ee2bb85. The PR was not finished yet, more commits were added later on.

rschu1ze mentioned this pull request Jul 14, 2024

OpenSSL: Minor follow-up to #66064 ClickHouse/ClickHouse#66489

Merged

mattcaswell mentioned this pull request Aug 16, 2024

ThreadSanitizer reports data race in SSL_do_handshake #25202

Closed

vavroch2010 moved this from Waiting Review to Done in Development Board Mar 2, 2025

behcetm mentioned this pull request May 30, 2025

SIGSEGV in ossl_method_store_do_all #27726

Closed

nhorman mentioned this pull request Oct 8, 2025

Copy impls stack when calling ossl_method_store_do_all #28783

Closed

Navigation Menu

Uh oh!

Uh oh!

read lock store on ossl_method_store_do_all - #24782

read lock store on ossl_method_store_do_all#24782
nhorman wants to merge 5 commits into
openssl:masterfrom
nhorman:24672

nhorman commented Jul 2, 2024 •

edited by t8m

Loading

Uh oh!

paulidale left a comment

Uh oh!

Uh oh!

paulidale commented Jul 2, 2024

Uh oh!

t8m commented Jul 3, 2024

Uh oh!

t8m commented Jul 3, 2024

Uh oh!

nhorman commented Jul 3, 2024

Uh oh!

t8m left a comment

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Jul 9, 2024

Uh oh!

t8m commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Uh oh!

Conversation

nhorman commented Jul 2, 2024 • edited by t8m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulidale commented Jul 2, 2024

Uh oh!

t8m commented Jul 3, 2024

Uh oh!

t8m commented Jul 3, 2024

Uh oh!

nhorman commented Jul 3, 2024

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Jul 9, 2024

Uh oh!

t8m commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nhorman commented Jul 2, 2024 •

edited by t8m

Loading