diff options
| author | Neil Horman <[email protected]> | 2026-09-03 22:50:20 +0200 |
|---|---|---|
| committer | Neil Horman <[email protected]> | 2026-09-11 22:50:54 +0200 |
| commit | f99bbcd5afff69840f2bfed92458cd5ac092cf91 (patch) | |
| tree | 0064dfa7620da14636ca721bffe363b117a53374 /include | |
| parent | Revert "Free connections before listeners in the radix test cleanup" (diff) | |
| download | openssl-f99bbcd5afff69840f2bfed92458cd5ac092cf91.tar.gz openssl-f99bbcd5afff69840f2bfed92458cd5ac092cf91.zip | |
Dont have dgram_pairs track bios, track bio_dgram_pair_st instead
@ifanzki reported an asan issue with the quic_radix_test in:
https://github.com/openssl/openssl/issues/32420
And we've tried fixing it a few different ways, with varying amounts of
success
This commit represents an alternative approach which I think is more
general to the bio_dgram_pair bio type.
The problem:
We have a race condition as reported above in which freeing one side of
a BIO dgram pair results in a use after free
The root cause:
BIO_new_bio_dgram_pair creates 2 dgram BIOS, each with a reference count
of one (which is sensible, as each bio has a single owner). However,
after creating the bios, it calls BIO_make_bio_pair(), which links the
two together. Under the covers in the bio_dgram_pair implementation,
each bio stores the pointer of its peer bio _without_ taking an
additional reference on the bio. This means that, once the first bio is
freed, the second bio may be doing subsequent io, which requires
dereferencing of the peer bio , which is already release,
leading to the use after free case observed.
Nominally this isn't an issue, as most code that uses BIO_dgram_pairs
are single threaded, and freeing of the component bios only occurs after
any i/o is already terminated, avoiding the observed UAF. However, the
QUIC radix test exercises the thread assisted mode of the quic stack,
and in so doing passes each half of a bio dgram pair to a separate
thread.
The fix:
Nominally we could resolve this by increasing the reference count on
each of the peer bios, and dropping that ref count when each is freed,
so that only the last to free actually deallocates both instances, but
this creates a chicken and egg issue. Since the the stored reference to
each bio lives in the dgram pair implementation, upping the refcount of
the bios prevents either bio from ever reaching the implementation free
code (dgram_pair_free)
So instead I'm proposing the following in this commit:
1) Instead of having the internal bio data track the peer bio, instead
have the internal data track a shared bio_dgram_peer_st structure
which is reference counted
2) When creating a pair, the pair only uses the data (ring buffers,
locks, etc), from the shared data, as its guaranteed to live until
the last owner of the pair either frees itself BIO_free, or destroys
the pair vi BIO_dgram_destroy_pair
3) If either side of the pair disconnects via the calls in (3), the
shared data state is updated to reflect that, and the remaining peer
triggers on that to get BROKEN_PIPE return codes
Also added tests from @bukka, as they provide more coverage than the
test I had in mind.
Fixes #32420
Reviewed-by: Jakub Zelenka <[email protected]>
Reviewed-by: Andrew Dinh <[email protected]>
MergeDate: Fri Sep 11 20:50:58 2026
(Merged from https://github.com/openssl/openssl/pull/32498)
Diffstat (limited to 'include')
| -rw-r--r-- | include/internal/threads_common.h | 2 |
1 files changed, 2 insertions, 0 deletions
diff --git a/include/internal/threads_common.h b/include/internal/threads_common.h index e32257d6301..c1da68a5dbd 100644 --- a/include/internal/threads_common.h +++ b/include/internal/threads_common.h @@ -14,9 +14,11 @@ #if defined(__clang__) && defined(__has_feature) #if __has_feature(thread_sanitizer) +#if !defined(__SANITIZE_THREAD__) #define __SANITIZE_THREAD__ #endif #endif +#endif #if defined(__SANITIZE_THREAD__) #include <sanitizer/tsan_interface.h> |
