Disable tracking during feature resolution.

This can cause deadlocks if the tracker uses reflection that trigger a build of feature extensions. PiperOrigin-RevId: 674006558
protocolbuffers · Sep 12, 2024 · 782e8ad · 782e8ad
1 parent f6fd9c8
commit 782e8ad
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 14 deletions.
diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel
@@ -657,6 +657,7 @@ cc_library(
         "@com_google_absl//absl/base",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/base:dynamic_annotations",
+        "@com_google_absl//absl/cleanup",
         "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/container:fixed_array",
         "@com_google_absl//absl/container:flat_hash_map",

diff --git a/src/google/protobuf/compiler/cpp/tracker.cc b/src/google/protobuf/compiler/cpp/tracker.cc
@@ -102,6 +102,11 @@ std::vector<Sub> GenerateTrackerCalls(
       // Emit(), we need to include a newline here so that the line that follows
       // the annotation is on its own line.
       call_str.push_back('\n');
+      if (enable_tracking) {
+        call_str =
+            absl::StrCat("if (::", ProtobufNamespace(opts),
+                         "::internal::cpp::IsTrackingEnabled()) ", call_str);
+      }
     }
 
     subs.push_back(

diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -39,6 +39,7 @@
 #include "absl/base/const_init.h"
 #include "absl/base/dynamic_annotations.h"
 #include "absl/base/thread_annotations.h"
+#include "absl/cleanup/cleanup.h"
 #include "absl/container/btree_map.h"
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
@@ -572,6 +573,19 @@ class FlatAllocatorImpl {
   TypeMap<IntT, T...> used_;
 };
 
+// Allows us to disable tracking in the current thread while certain build steps
+// are happening.
+bool& is_tracking_enabled() {
+  static PROTOBUF_THREAD_LOCAL bool value = true;
+  return value;
+}
+
+auto DisableTracking() {
+  bool old_value = is_tracking_enabled();
+  is_tracking_enabled() = false;
+  return absl::MakeCleanup([=] { is_tracking_enabled() = old_value; });
+}
+
 }  // namespace
 
 class Symbol {
@@ -6156,20 +6170,24 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
 
     // Handle feature resolution.  This must occur after option interpretation,
     // but before validation.
-    internal::VisitDescriptors(
-        *result, proto, [&](const auto& descriptor, const auto& proto) {
-          using OptionsT =
-              typename std::remove_const<typename std::remove_pointer<
-                  decltype(descriptor.options_)>::type>::type;
-          using DescriptorT = typename std::remove_const<
-              typename std::remove_reference<decltype(descriptor)>::type>::type;
-
-          ResolveFeatures(
-              proto, const_cast<DescriptorT*>(&descriptor),
-              const_cast<  // NOLINT(google3-runtime-proto-const-cast)
-                  OptionsT*>(descriptor.options_),
-              alloc);
-        });
+    {
+      auto cleanup = DisableTracking();
+      internal::VisitDescriptors(
+          *result, proto, [&](const auto& descriptor, const auto& proto) {
+            using OptionsT =
+                typename std::remove_const<typename std::remove_pointer<
+                    decltype(descriptor.options_)>::type>::type;
+            using DescriptorT =
+                typename std::remove_const<typename std::remove_reference<
+                    decltype(descriptor)>::type>::type;
+
+            ResolveFeatures(
+                proto, const_cast<DescriptorT*>(&descriptor),
+                const_cast<  // NOLINT(google3-runtime-proto-const-cast)
+                    OptionsT*>(descriptor.options_),
+                alloc);
+          });
+    }
 
     internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) {
       if (result->edition() >= Edition::EDITION_2024 &&
@@ -9782,6 +9800,8 @@ void LazyDescriptor::Once(const ServiceDescriptor* service) {
 }
 
 bool ParseNoReflection(absl::string_view from, google::protobuf::MessageLite& to) {
+  auto cleanup = DisableTracking();
+
   to.Clear();
   const char* ptr;
   internal::ParseContext ctx(io::CodedInputStream::GetDefaultRecursionLimit(),
@@ -9854,6 +9874,8 @@ bool IsLazilyInitializedFile(absl::string_view filename) {
          filename == "google/protobuf/descriptor.proto";
 }
 
+bool IsTrackingEnabled() { return is_tracking_enabled(); }
+
 }  // namespace cpp
 }  // namespace internal
 

diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h
@@ -2983,6 +2983,11 @@ PROTOBUF_EXPORT bool IsGroupLike(const FieldDescriptor& field);
 // protos to avoid linker bloat in lite runtimes.
 PROTOBUF_EXPORT bool IsLazilyInitializedFile(absl::string_view filename);
 
+// Returns true during internal calls that should avoid calling trackers.  These
+// calls can be particularly dangerous during build steps like feature
+// resolution, where a MergeFrom call can wind up in a deadlock.
+PROTOBUF_EXPORT bool IsTrackingEnabled();
+
 template <typename F>
 auto VisitDescriptorsInFileOrder(const Descriptor* desc,
                                  F& f) -> decltype(f(desc)) {