From 6e8b9e4a4a478c99a93925035b004a3829fd1e4f Mon Sep 17 00:00:00 2001 From: "kenton@google.com" Date: Tue, 22 Dec 2009 23:51:20 +0000 Subject: [PATCH] Make extension identifiers final. This improves security when untrusted code is present in the same class loader. In order to get around initialization ordering issues, I simply made the constructor for extension identifiers take no arguments and deferred initialization to an internalInit() method, which generated code will always call during init. --- .../com/google/protobuf/GeneratedMessage.java | 50 ++++++------ .../google/protobuf/GeneratedMessageLite.java | 78 +++++++++++-------- .../protobuf/compiler/java/java_extension.cc | 62 +++++++-------- 3 files changed, 95 insertions(+), 95 deletions(-) diff --git a/java/src/main/java/com/google/protobuf/GeneratedMessage.java b/java/src/main/java/com/google/protobuf/GeneratedMessage.java index 47652e546f..dba0ec83c9 100644 --- a/java/src/main/java/com/google/protobuf/GeneratedMessage.java +++ b/java/src/main/java/com/google/protobuf/GeneratedMessage.java @@ -724,25 +724,8 @@ public abstract class GeneratedMessage extends AbstractMessage { /** For use by generated code only. */ public static GeneratedExtension - newGeneratedExtension(final FieldDescriptor descriptor, - final Class type) { - if (descriptor.isRepeated()) { - throw new IllegalArgumentException( - "Must call newRepeatedGeneratedExtension() for repeated types."); - } - return new GeneratedExtension(descriptor, type); - } - - /** For use by generated code only. */ - public static - GeneratedExtension> - newRepeatedGeneratedExtension( - final FieldDescriptor descriptor, final Class type) { - if (!descriptor.isRepeated()) { - throw new IllegalArgumentException( - "Must call newGeneratedExtension() for non-repeated types."); - } - return new GeneratedExtension>(descriptor, type); + newGeneratedExtension() { + return new GeneratedExtension(); } /** @@ -775,8 +758,23 @@ public abstract class GeneratedMessage extends AbstractMessage { // TODO(kenton): Find ways to avoid using Java reflection within this // class. Also try to avoid suppressing unchecked warnings. - private GeneratedExtension(final FieldDescriptor descriptor, - final Class type) { + // We can't always initialize a GeneratedExtension when we first construct + // it due to initialization order difficulties (namely, the descriptor may + // not have been constructed yet, since it is often constructed by the + // initializer of a separate module). So, we construct an uninitialized + // GeneratedExtension once, then call internalInit() on it later. Generated + // code will always call internalInit() on all extensions as part of the + // static initialization code, and internalInit() throws an exception if + // called more than once, so this method is useless to users. + private GeneratedExtension() {} + + /** For use by generated code only. */ + public void internalInit(final FieldDescriptor descriptor, + final Class type) { + if (this.descriptor != null) { + throw new IllegalStateException("Already initialized."); + } + if (!descriptor.isExtension()) { throw new IllegalArgumentException( "GeneratedExtension given a regular (non-extension) field."); @@ -811,11 +809,11 @@ public abstract class GeneratedMessage extends AbstractMessage { } } - private final FieldDescriptor descriptor; - private final Class type; - private final Method enumValueOf; - private final Method enumGetValueDescriptor; - private final Message messageDefaultInstance; + private FieldDescriptor descriptor; + private Class type; + private Method enumValueOf; + private Method enumGetValueDescriptor; + private Message messageDefaultInstance; public FieldDescriptor getDescriptor() { return descriptor; } diff --git a/java/src/main/java/com/google/protobuf/GeneratedMessageLite.java b/java/src/main/java/com/google/protobuf/GeneratedMessageLite.java index e327f74544..9cdd4e94d1 100644 --- a/java/src/main/java/com/google/protobuf/GeneratedMessageLite.java +++ b/java/src/main/java/com/google/protobuf/GeneratedMessageLite.java @@ -420,34 +420,8 @@ public abstract class GeneratedMessageLite extends AbstractMessageLite { /** For use by generated code only. */ public static GeneratedExtension - newGeneratedExtension( - final ContainingType containingTypeDefaultInstance, - final Type defaultValue, - final MessageLite messageDefaultInstance, - final Internal.EnumLiteMap enumTypeMap, - final int number, - final WireFormat.FieldType type) { - return new GeneratedExtension( - containingTypeDefaultInstance, defaultValue, messageDefaultInstance, - new ExtensionDescriptor(enumTypeMap, number, type, - false /* isRepeated */, false /* isPacked */)); - } - - /** For use by generated code only. */ - public static - GeneratedExtension> - newRepeatedGeneratedExtension( - final ContainingType containingTypeDefaultInstance, - final MessageLite messageDefaultInstance, - final Internal.EnumLiteMap enumTypeMap, - final int number, - final WireFormat.FieldType type, - final boolean isPacked) { - return new GeneratedExtension>( - containingTypeDefaultInstance, Collections.emptyList(), - messageDefaultInstance, - new ExtensionDescriptor( - enumTypeMap, number, type, true /* isRepeated */, isPacked)); + newGeneratedExtension() { + return new GeneratedExtension(); } private static final class ExtensionDescriptor @@ -515,7 +489,16 @@ public abstract class GeneratedMessageLite extends AbstractMessageLite { */ public static final class GeneratedExtension< ContainingType extends MessageLite, Type> { - private GeneratedExtension( + // We can't always initialize a GeneratedExtension when we first construct + // it due to initialization order difficulties (namely, the default + // instances may not have been constructed yet). So, we construct an + // uninitialized GeneratedExtension once, then call internalInit() on it + // later. Generated code will always call internalInit() on all extensions + // as part of the static initialization code, and internalInit() throws an + // exception if called more than once, so this method is useless to users. + private GeneratedExtension() {} + + private void internalInit( final ContainingType containingTypeDefaultInstance, final Type defaultValue, final MessageLite messageDefaultInstance, @@ -526,10 +509,39 @@ public abstract class GeneratedMessageLite extends AbstractMessageLite { this.descriptor = descriptor; } - private final ContainingType containingTypeDefaultInstance; - private final Type defaultValue; - private final MessageLite messageDefaultInstance; - private final ExtensionDescriptor descriptor; + /** For use by generated code only. */ + public void internalInitSingular( + final ContainingType containingTypeDefaultInstance, + final Type defaultValue, + final MessageLite messageDefaultInstance, + final Internal.EnumLiteMap enumTypeMap, + final int number, + final WireFormat.FieldType type) { + internalInit( + containingTypeDefaultInstance, defaultValue, messageDefaultInstance, + new ExtensionDescriptor(enumTypeMap, number, type, + false /* isRepeated */, false /* isPacked */)); + } + + /** For use by generated code only. */ + public void internalInitRepeated( + final ContainingType containingTypeDefaultInstance, + final MessageLite messageDefaultInstance, + final Internal.EnumLiteMap enumTypeMap, + final int number, + final WireFormat.FieldType type, + final boolean isPacked) { + internalInit( + containingTypeDefaultInstance, (Type) Collections.emptyList(), + messageDefaultInstance, + new ExtensionDescriptor( + enumTypeMap, number, type, true /* isRepeated */, isPacked)); + } + + private ContainingType containingTypeDefaultInstance; + private Type defaultValue; + private MessageLite messageDefaultInstance; + private ExtensionDescriptor descriptor; /** * Default instance of the type being extended, used to identify that type. diff --git a/src/google/protobuf/compiler/java/java_extension.cc b/src/google/protobuf/compiler/java/java_extension.cc index 5932433f15..903b0a9bdf 100644 --- a/src/google/protobuf/compiler/java/java_extension.cc +++ b/src/google/protobuf/compiler/java/java_extension.cc @@ -112,16 +112,20 @@ void ExtensionGenerator::Generate(io::Printer* printer) { "public static final int $constant_name$ = $number$;\n"); if (descriptor_->is_repeated()) { printer->Print(vars, - "public static\n" + "public static final\n" " com.google.protobuf.GeneratedMessage$lite$.GeneratedExtension<\n" " $containing_type$,\n" - " java.util.List<$type$>> $name$;\n"); + " java.util.List<$type$>> $name$ =\n" + " com.google.protobuf.GeneratedMessage$lite$\n" + " .newGeneratedExtension();\n"); } else { printer->Print(vars, - "public static\n" + "public static final\n" " com.google.protobuf.GeneratedMessage$lite$.GeneratedExtension<\n" " $containing_type$,\n" - " $type$> $name$;\n"); + " $type$> $name$ =\n" + " com.google.protobuf.GeneratedMessage$lite$\n" + " .newGeneratedExtension();\n"); } } @@ -157,43 +161,29 @@ void ExtensionGenerator::GenerateInitializationCode(io::Printer* printer) { } if (HasDescriptorMethods(descriptor_->file())) { - if (descriptor_->is_repeated()) { - printer->Print(vars, - "$scope$.$name$ =\n" - " com.google.protobuf.GeneratedMessage\n" - " .newRepeatedGeneratedExtension(\n" - " $scope$.getDescriptor().getExtensions().get($index$),\n" - " $type$.class);\n"); - } else { - printer->Print(vars, - "$scope$.$name$ =\n" - " com.google.protobuf.GeneratedMessage.newGeneratedExtension(\n" - " $scope$.getDescriptor().getExtensions().get($index$),\n" - " $type$.class);\n"); - } + printer->Print(vars, + "$scope$.$name$.internalInit(\n" + " $scope$.getDescriptor().getExtensions().get($index$),\n" + " $type$.class);\n"); } else { if (descriptor_->is_repeated()) { printer->Print(vars, - "$scope$.$name$ =\n" - " com.google.protobuf.GeneratedMessageLite\n" - " .newRepeatedGeneratedExtension(\n" - " $extendee$.getDefaultInstance(),\n" - " $prototype$,\n" - " $enum_map$,\n" - " $number$,\n" - " com.google.protobuf.WireFormat.FieldType.$type_constant$,\n" - " $packed$);\n"); + "$scope$.$name$.internalInitRepeated(\n" + " $extendee$.getDefaultInstance(),\n" + " $prototype$,\n" + " $enum_map$,\n" + " $number$,\n" + " com.google.protobuf.WireFormat.FieldType.$type_constant$,\n" + " $packed$);\n"); } else { printer->Print(vars, - "$scope$.$name$ =\n" - " com.google.protobuf.GeneratedMessageLite\n" - " .newGeneratedExtension(\n" - " $extendee$.getDefaultInstance(),\n" - " $default$,\n" - " $prototype$,\n" - " $enum_map$,\n" - " $number$,\n" - " com.google.protobuf.WireFormat.FieldType.$type_constant$);\n"); + "$scope$.$name$.internalInitSingular(\n" + " $extendee$.getDefaultInstance(),\n" + " $default$,\n" + " $prototype$,\n" + " $enum_map$,\n" + " $number$,\n" + " com.google.protobuf.WireFormat.FieldType.$type_constant$);\n"); } } }