Upstream merge 2023 08 09 #1152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justsmth
force-pushed
the
upstream-merge-2023-08-09
branch
from
8dd7c4f to
2d8a86e
Compare
dkostic
reviewed
Aug 16, 2023
billbo-yang
reviewed
Aug 16, 2023
Change-Id: Ibcc3faa4c3e03713a0c8f6dc24119e4579a5b4e4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59025 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]> (cherry picked from commit 706846d7a84c3ce8d089b7089c298bd7449227b3)
Right now, MSVC has to fallback to refcount_lock.c, which uses a single, global lock for all refcount operations. Instead, use the Interlocked* APIs to implement them. The motivation is two-fold. First, this removes a performance cliff when building for Windows on a non-Clang compiler. (Although I've not been able to measure it in an end-to-end EVP benchmark, only a synthetic refcount-only benchmark.) More importantly, it gets us closer to assuming atomics support on all non-NO_THREADS configurations. (The next CL will clear through that.) That, in turn, will make it easier to add an atomics-like abstractions to some of our hotter synchronization points. (Even in newer glibc, with its better rwlock, read locks fundamentally need to write to memory, so we have some cacheline contention on shared locks.) Annoyingly, the Windows atomic_load replacement is not quite right. I've used a "no-op" InterlockedCompareExchange(p, 0, 0) which, empirically, still results in a write. But a write to the refcount cacheline is surely better than taking a global exclusive lock. See comments in file for details. OpenSSL uses InterlockedOr(p, 0), but that actually results in even worse code. (InterlockedOr needs a retry loop when the underlying cmpxchg fails, whereas InterlockedCompareExchange is a single cmpxchg.) Hopefully, in the future (perhaps when we require VS 2022's successor, based on [1]), this can be removed in favor of C11 atomics everywhere. [1] https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ Bug: 570 Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I125da139e2fd3ae51e54309309fda16ba97ccf20 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59846 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit 5b845de636224ef3e065be8e1c7d2df3389aa175)
This is intended to be run under TSan. ex_data currently works by taking a global read lock every time an RSA, SSL, SSL_CTX, etc., is freed. We should be able to fix that but, first, make sure we have test coverage for the threading requirements. Bug: 570 Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I0e12907e116481d88e45191a1f15f3a51833bf6d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59865 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 2eaf07075ac15491c35e1ba1e836797fa81eb96b)
If a process calls fork(), then the child process never forks again, the child may wish to call RAND_enable_fork_unsafe_buffering(). However, doing so exposes a bug: we assume that, if the flag is set, we don't need to worry about fork-safety. But it is possible that the PRNG state was cloned from another process which does not work. Concretely, consider a zygote process, e.g. Chromium's. A zygote process would retain fork-safety, but pass along its PRNG state to each of its children. If the children never fork, they might disable fork-safety, hitting this bug. (Chromium does not call this API. This is just a hypothetical scenario.) Fix this by reseeding whenever the fork-safety bit changes. This fix does not strictly depend on the atomics work, but it causes us to unconditionally sample rand_fork_unsafe_buffering_enabled(). This no longer causes contention because it's just another atomic load. This only affects systems without MADV_WIPEONFORK and without fast RDRAND. If RDRAND is fast, we're always fork-safe and MADV_WIPEONFORK allows us to efficiently detect forks. Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I6d0c471c62c951254faf85420a7dc3f4a9d65ee0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59850 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit dd5219451c3ce26221762a15d867edf43b463bb2)
Change-Id: I190fcdb0b9667b0ac6f490b36edc63237af7fffb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59905 Reviewed-by: David Benjamin <[email protected]> (cherry picked from commit 47b2fefb03122e49375a252698c857e477c8cf35)
https://crrev.com/c/4415225 had to react to us querying these. Add a comment so we can fix this proactively in the future. Bug: chromium:1432323 Change-Id: I2ffe4d90e32215b521815a25f3448502da2156bf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59925 Reviewed-by: Adam Langley <[email protected]> (cherry picked from commit 74a75b39596757546c1070f3dc5184bbab44397b)
Although we've switched STACK_OF(T) to use size_t, OpenSSL used int pervasively. Much of crypto/x509 and third-party callers use int indices. As much of that is in the public API now, ensure that STACK_OF(T) can never exceed INT_MAX elements. Bug: 516 Change-Id: I26b8fe590655f8c3e449b749b5d0222e28c413f8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60065 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> (cherry picked from commit 8c7e925b5dab1f826f08f38e4b9e1543b8413476)
The Close() method of the middle often wasn't getting called because `os.Exit(0)` was used in some places. Once that's fixed, it's clear that the queue of pending reads needed to be closed before waiting for the reader goroutine to finish. Lastly, don't bother trying to record the error that the reader saw: just panic the process if the modulewrapper dies during processing. Change-Id: Icf077cefd0ace2ef721a493f99fede6269531257 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60045 Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> (cherry picked from commit cf3851c6c9380368373ac127cde1f4aa7159fba3)
justsmth
force-pushed
the
upstream-merge-2023-08-09
branch
from
2d8a86e to
15b349a
Compare
billbo-yang
approved these changes
Aug 16, 2023
torben-hansen
approved these changes
Aug 16, 2023
dkostic
approved these changes
Aug 16, 2023
justsmth
added a commit
to justsmth/aws-lc
that referenced
this pull request
Aug 17, 2023
This reverts commit bd75ca5.
justsmth
added a commit
that referenced
this pull request
Aug 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Merge upstream changes from google/boringssl@706846d to google/boringssl@8c7e925
Call-outs:
See internal document.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.