From fbf6b395c830edd7ade1d0042058e94531769962 Mon Sep 17 00:00:00 2001 From: Zygo Blaxell Date: Sat, 11 Dec 2021 15:35:50 -0500 Subject: [PATCH] types: member m_fd in BeesFileRange must be protected against data races We had an unfortunate pattern of: const BeesFileRange bfr; shared_ptr ctx; // ... BEESNOTE("foo " << bfr); bfr.fd(ctx); BEESNOTE("foo after opening: " << bfr); If dump_status started running after the first BEESNOTE, but before the second, then bfr.fd() might expose a single Fd object's shared_ptr member to two threads at the same time (the thread running dump_status and the thread running BEESNOTE) without protection by a lock. One of the threads would see a partially-initialized Fd object, and the other thread would crash on an assertion failure, e.g. #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007f4c4fde5537 in __GI_abort () at abort.c:79 #2 0x00007f4c4fde540f in __assert_fail_base (fmt=0x7f4c4ff4e128 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5557605629dd "!m_destroyed", file=0x5557605627c0 "../include/crucible/namedptr.h", line=77, function=) at assert.c:92 #3 0x00007f4c4fdf4662 in __GI___assert_fail (assertion=assertion@entry=0x5557605629dd "!m_destroyed", file=file@entry=0x5557605627c0 "../include/crucible/namedptr.h", line=line@entry=77, function=function@entry=0x555760562970 "crucible::NamedPtr::Value::~Value() [with Return = crucible::IOHandle; Arguments = {int}]") at assert.c:101 #4 0x00005557605306f6 in crucible::NamedPtr::Value::~Value (this=0x7f4a3c2ff0d0, __in_chrg=) at ../include/crucible/namedptr.h:77 #5 0x00005557605137da in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x7f4a3c2ff0c0) at /usr/include/c++/10/bits/shared_ptr_base.h:151 #6 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x7f4a3c2ff0c0) at /usr/include/c++/10/bits/shared_ptr_base.h:151 #7 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x7f4c4c5b5f28, __in_chrg=) at /usr/include/c++/10/bits/shared_ptr_base.h:733 #8 std::__shared_ptr::~__shared_ptr (this=0x7f4c4c5b5f20, __in_chrg=) at /usr/include/c++/10/bits/shared_ptr_base.h:1183 #9 std::shared_ptr::~shared_ptr (this=0x7f4c4c5b5f20, __in_chrg=) at /usr/include/c++/10/bits/shared_ptr.h:121 #10 crucible::Fd::~Fd (this=0x7f4c4c5b5f20, __in_chrg=) at ../include/crucible/fd.h:46 #11 BeesFileRange::file_size (this=0x7f4c4e5ba4a0) at bees-types.cc:156 #12 0x0000555760513950 in operator<< (os=..., bfr=...) at bees-types.cc:80 #13 0x000055576050d662 in std::function::operator()(std::ostream&) const (__args#0=..., this=0x7f4c4e5b9f60) at /usr/include/c++/10/bits/std_function.h:622 #14 BeesNote::get_status[abi:cxx11]() () at bees-trace.cc:165 #15 0x00005557604c9676 in BeesContext::dump_status (this=0x5557611c4de0) at bees-context.cc:89 #16 0x00005557605206fb in std::function::operator()() const (this=this@entry=0x7f4c4c5b65f0) at /usr/include/c++/10/bits/std_function.h:622 #17 crucible::catch_all(std::function const&, std::function, std::allocator >)> const&) (f=..., explainer=...) at error.cc:55 #18 0x000055576050aaa7 in operator() (__closure=0x5557611c52c8) at bees-thread.cc:22 #19 0x00007f4c501beed0 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6 #20 0x00007f4c502c8ea7 in start_thread (arg=) at pthread_create.c:477 #21 0x00007f4c4febddef in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 Fix by making BeesFileRange::m_fd really const (not just mutable), then fix all the broken code referencing it. Signed-off-by: Zygo Blaxell --- src/bees-context.cc | 25 ++++++++++++------------- src/bees-resolve.cc | 16 +++++++++------- src/bees-types.cc | 2 +- src/bees.h | 4 ++-- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/bees-context.cc b/src/bees-context.cc index f805625..0f3c629 100644 --- a/src/bees-context.cc +++ b/src/bees-context.cc @@ -187,20 +187,20 @@ BeesContext::is_root_ro(uint64_t root) } bool -BeesContext::dedup(const BeesRangePair &brp) +BeesContext::dedup(const BeesRangePair &brp_in) { // TOOLONG and NOTE can retroactively fill in the filename details, but LOG can't - BEESNOTE("dedup " << brp); + BEESNOTE("dedup " << brp_in); - brp.second.fd(shared_from_this()); - - if (is_root_ro(brp.second.fid().root())) { - // BEESLOGDEBUG("WORKAROUND: dst root is read-only in " << name_fd(brp.second.fd())); + if (is_root_ro(brp_in.second.fid().root())) { + // BEESLOGDEBUG("WORKAROUND: dst root " << (brp_in.second.fid().root()) << " is read-only); BEESCOUNT(dedup_workaround_btrfs_send); return false; } + auto brp = brp_in; brp.first.fd(shared_from_this()); + brp.second.fd(shared_from_this()); BEESTOOLONG("dedup " << brp); @@ -725,23 +725,22 @@ BeesContext::scan_one_extent(const BeesFileRange &bfr, const Extent &e) } BeesFileRange -BeesContext::scan_forward(const BeesFileRange &bfr) +BeesContext::scan_forward(const BeesFileRange &bfr_in) { - // What are we doing here? - BEESTRACE("scan_forward " << bfr); + BEESTRACE("scan_forward " << bfr_in); BEESCOUNT(scan_forward); Timer scan_timer; // Silently filter out blacklisted files - if (is_blacklisted(bfr.fid())) { + if (is_blacklisted(bfr_in.fid())) { BEESCOUNT(scan_blacklisted); - return bfr; + return bfr_in; } - BEESNOTE("scan open " << bfr); - // Reconstitute FD + BEESNOTE("scan open " << bfr_in); + auto bfr = bfr_in; bfr.fd(shared_from_this()); BEESNOTE("scan extent " << bfr); diff --git a/src/bees-resolve.cc b/src/bees-resolve.cc index 66608b7..0d4d2dc 100644 --- a/src/bees-resolve.cc +++ b/src/bees-resolve.cc @@ -385,14 +385,15 @@ BeesResolver::for_each_extent_ref(BeesBlockData bbd, function bool { + for_each_extent_ref(bbd, [&](const BeesFileRange &src_bfr_in) -> bool { // Open src - BEESNOTE("Opening src bfr " << src_bfr); - BEESTRACE("Opening src bfr " << src_bfr); + BEESNOTE("Opening src bfr " << src_bfr_in); + BEESTRACE("Opening src bfr " << src_bfr_in); + auto src_bfr = src_bfr_in; src_bfr.fd(m_ctx); if (dst_bfr.overlaps(src_bfr)) { diff --git a/src/bees-types.cc b/src/bees-types.cc index f3f1c86..23c3146 100644 --- a/src/bees-types.cc +++ b/src/bees-types.cc @@ -287,7 +287,7 @@ BeesFileRange::fd() const } Fd -BeesFileRange::fd(const shared_ptr &ctx) const +BeesFileRange::fd(const shared_ptr &ctx) { // If we don't have a fid we can't do much here if (m_fid) { diff --git a/src/bees.h b/src/bees.h index 3d920f8..6d7a66e 100644 --- a/src/bees.h +++ b/src/bees.h @@ -260,7 +260,7 @@ ostream& operator<<(ostream &os, const BeesFileId &bfi); class BeesFileRange { protected: - mutable Fd m_fd; + Fd m_fd; mutable BeesFileId m_fid; off_t m_begin = 0, m_end = 0; mutable off_t m_file_size = -1; @@ -301,7 +301,7 @@ public: Fd fd() const; // Get the fd, opening it if necessary - Fd fd(const shared_ptr &ctx) const; + Fd fd(const shared_ptr &ctx); BeesFileRange copy_closed() const;