From 7f0787829fd677a5d0bbaf0cdaf10c29b76c9b2b Mon Sep 17 00:00:00 2001
From: Protobuf Team Bot <protobuf-github-bot@google.com>
Date: Wed, 18 Oct 2023 11:29:11 -0700
Subject: [PATCH] Clarify the preconditions of AddXXX functions. Also, add a
 debug check to verify this precondition.

PiperOrigin-RevId: 574538974
---
 src/google/protobuf/repeated_field_unittest.cc | 8 ++++++++
 src/google/protobuf/repeated_ptr_field.h       | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/src/google/protobuf/repeated_field_unittest.cc b/src/google/protobuf/repeated_field_unittest.cc
index f5529ed784..529d872284 100644
--- a/src/google/protobuf/repeated_field_unittest.cc
+++ b/src/google/protobuf/repeated_field_unittest.cc
@@ -1677,6 +1677,14 @@ TEST(RepeatedPtrField, AddAllocated) {
   EXPECT_EQ(moo, &field.Get(index));
 }
 
+TEST(RepeatedPtrField, AddMethodsDontAcceptNull) {
+#if !defined(NDEBUG)
+  RepeatedPtrField<std::string> field;
+  EXPECT_DEATH(field.AddAllocated(nullptr), "nullptr");
+  EXPECT_DEATH(field.UnsafeArenaAddAllocated(nullptr), "nullptr");
+#endif
+}
+
 TEST(RepeatedPtrField, AddAllocatedDifferentArena) {
   RepeatedPtrField<TestAllTypes> field;
   Arena arena;
diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h
index 08642b56c0..72beb1c2f5 100644
--- a/src/google/protobuf/repeated_ptr_field.h
+++ b/src/google/protobuf/repeated_ptr_field.h
@@ -443,11 +443,13 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
   template <typename TypeHandler>
   void AddAllocated(Value<TypeHandler>* value) {
     typename TypeImplementsMergeBehavior<Value<TypeHandler>>::type t;
+    ABSL_DCHECK_NE(value, nullptr);
     AddAllocatedInternal<TypeHandler>(value, t);
   }
 
   template <typename TypeHandler>
   void UnsafeArenaAddAllocated(Value<TypeHandler>* value) {
+    ABSL_DCHECK_NE(value, nullptr);
     // Make room for the new pointer.
     if (current_size_ == total_size_) {
       // The array is completely full with no cleared objects, so grow it.
@@ -1179,6 +1181,7 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
   //   (ii) if this field holds strings, the passed-in string *must* be
   //   heap-allocated, not arena-allocated. There is no way to dynamically check
   //   this at runtime, so User Beware.
+  // Requires:  value != nullptr
   void AddAllocated(Element* value);
 
   // Removes and returns the last element, passing ownership to the caller.
@@ -1201,6 +1204,7 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
   // If you put temp_field on the arena this fails, because the ownership
   // transfers to the arena at the "AddAllocated" call and is not released
   // anymore, causing a double delete. UnsafeArenaAddAllocated prevents this.
+  // Requires:  value != nullptr
   void UnsafeArenaAddAllocated(Element* value);
 
   // Removes and returns the last element.  Unlike ReleaseLast, the returned