Change pre-22 poison pill to only log once per affected message type. (#21754)

Author: esrauchg

java/core/src/main/java/com/google/protobuf/GeneratedMessage.java

@@ -21,9 +21,11 @@
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
 import java.util.logging.Logger;
 
@@ -329,21 +331,30 @@ public int getSerializedSize() {
           + " security vulnerability:"
           + " https://github.com/protocolbuffers/protobuf/security/advisories/GHSA-h4h5-3hr4-j3g2";
 
-  static void warnPre22Gencode() {
+  private static final Set<String> loggedPre22TypeNames
+      = Collections.synchronizedSet(new HashSet<String>());
+  static void warnPre22Gencode(Class<?> messageClass) {
     if (System.getProperty(PRE22_GENCODE_SILENCE_PROPERTY) != null) {
       return;
     }
-    UnsupportedOperationException exception =
-        new UnsupportedOperationException(PRE22_GENCODE_VULNERABILITY_MESSAGE);
+    String messageName = messageClass.getName();
+    String vulnerabilityMessage =
+          "Vulnerable protobuf generated type in use: " + messageName + "\n" +
+          PRE22_GENCODE_VULNERABILITY_MESSAGE;
+
     if (System.getProperty(PRE22_GENCODE_ERROR_PROPERTY) != null) {
-      throw exception;
+      throw new UnsupportedOperationException(vulnerabilityMessage);
+    }
+
+    if (!loggedPre22TypeNames.add(messageName)) {
+      return;
     }
-    logger.warning(exception.toString());
+    logger.warning(vulnerabilityMessage);
   }
 
   /** Used by parsing constructors in generated classes. */
   protected void makeExtensionsImmutable() {
-    warnPre22Gencode();
+    warnPre22Gencode(getClass());
   }
 
   /**
@@ -933,7 +944,7 @@ protected boolean parseUnknownField(
     /** Used by parsing constructors in generated classes. */
     @Override
     protected void makeExtensionsImmutable() {
-      warnPre22Gencode();
+      warnPre22Gencode(getClass());
       extensions.makeImmutable();
     }
 

java/core/src/main/java/com/google/protobuf/GeneratedMessageV3.java

@@ -528,7 +528,7 @@ protected Object newInstance(UnusedPrivateParameter unused) {
    */
   protected void makeExtensionsImmutable() {
     // Noop for messages without extensions.
-    GeneratedMessage.warnPre22Gencode();
+    GeneratedMessage.warnPre22Gencode(getClass());
   }
 
   /**
@@ -1276,7 +1276,7 @@ protected boolean parseUnknownFieldProto3(
      */
     @Override
     protected void makeExtensionsImmutable() {
-      GeneratedMessage.warnPre22Gencode();
+      GeneratedMessage.warnPre22Gencode(getClass());
       extensions.makeImmutable();
     }
 

java/core/src/test/java/com/google/protobuf/GeneratedMessageTest.java

@@ -2015,39 +2015,58 @@ private TestUtil.TestLogHandler setupLogger() {
   @Test
   public void generatedMessage_makeExtensionsImmutableShouldLog() {
     TestUtil.TestLogHandler logHandler = setupLogger();
-    GeneratedMessageV3 msg =
-        new GeneratedMessageV3() {
-          @Override
-          protected FieldAccessorTable internalGetFieldAccessorTable() {
-            return null;
-          }
-
-          @Override
-          protected Message.Builder newBuilderForType(BuilderParent parent) {
-            return null;
-          }
+    class TestMessage1 extends GeneratedMessageV3 {
+        @Override
+        protected FieldAccessorTable internalGetFieldAccessorTable() {
+          return null;
+        }
+
+        @Override
+        protected Message.Builder newBuilderForType(BuilderParent parent) {
+          return null;
+        }
+
+        @Override
+        public Message.Builder newBuilderForType() {
+          return null;
+        }
+
+        @Override
+        public Message.Builder toBuilder() {
+          return null;
+        }
+
+        @Override
+        public Message getDefaultInstanceForType() {
+          return null;
+        }
+    }
 
-          @Override
-          public Message.Builder newBuilderForType() {
-            return null;
-          }
+    class TestMessage2 extends TestMessage1 {}
 
-          @Override
-          public Message.Builder toBuilder() {
-            return null;
-          }
-
-          @Override
-          public Message getDefaultInstanceForType() {
-            return null;
-          }
-        };
+    TestMessage1 msg = new TestMessage1();
     msg.makeExtensionsImmutable();
     List<LogRecord> logs = logHandler.getStoredLogRecords();
     assertThat(logs).hasSize(1);
     String message = logs.get(0).getMessage();
+    // The generated type
+    assertThat(message).contains(
+        "Vulnerable protobuf generated type in use: " +
+        "com.google.protobuf.GeneratedMessageTest$1TestMessage1");
     assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
     assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY);
+
+    // Subsequent calls for the same type do not log again.
+    msg.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).hasSize(1);
+
+    // A call on a second type does log for that type.
+    TestMessage2 msg2 = new TestMessage2();
+    msg2.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).hasSize(2);
+    // And not again (only once per type).
+    msg2.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).hasSize(2);
   }
 
   @Test
@@ -2059,7 +2078,14 @@ public void extendableMessage_makeExtensionsImmutableShouldThrow() {
     List<LogRecord> logs = logHandler.getStoredLogRecords();
     assertThat(logs).hasSize(1);
     String message = logs.get(0).getMessage();
+    assertThat(message).contains(
+      "Vulnerable protobuf generated type in use: " +
+      "protobuf_unittest.UnittestProto$TestAllExtensions");
     assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_VULNERABILITY_MESSAGE);
     assertThat(message).contains(GeneratedMessage.PRE22_GENCODE_SILENCE_PROPERTY);
+
+    // Subsequent calls for the same type do not log again.
+    msg.makeExtensionsImmutable();
+    assertThat(logHandler.getStoredLogRecords()).hasSize(1);
   }
 }