Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE #24

protobufel · 2014-09-16T22:06:35Z

Here is the excerpt from Descriptors$FieldDescriptor constructor:

    private FieldDescriptor(final FieldDescriptorProto proto,
                            final FileDescriptor file,
                            final Descriptor parent,
                            final int index,
                            final boolean isExtension)
                     throws DescriptorValidationException {
      this.index = index;
      this.proto = proto;
      fullName = computeFullName(file, parent, proto.getName());
      this.file = file;

      if (proto.hasType()) {
        type = Type.valueOf(proto.getType());
      }

      if (getNumber() <= 0) {
        throw new DescriptorValidationException(this,
          "Field numbers must be positive integers.");
      }

      // Only repeated primitive fields may be packed.
      if (proto.getOptions().getPacked() && !isPackable()) {

... 

    public boolean isPackable() {
      return isRepeated() && getLiteType().isPackable();
    }

    public WireFormat.FieldType getLiteType() {
      return table[type.ordinal()];
    }

However, if there is an enum or a message proto field with unresolved type (which is okay during cross-linking), then isPackable() will NPE because type ==null in getLiteType() !

This is a serious bug, preventing build of ANY protos with enum or message fields with only the TypeName set. The protoc does its own cross-linking so its Type fields are always set, anything else will fail miserably!

The resolution is simple add type != null in this condition, and if wanted check again after cross-linking:

if ((type != null) && proto.getOptions().getPacked() && !isPackable()) {

Thanks in advance, and hope it will be released soon!

Regards,
David

protobufel · 2014-09-17T00:36:57Z

And here is the test for this case:

  @Test(expected = NullPointerException.class)
  public void testPackedEnumFieldInProto() throws Exception {
    final FileDescriptorProto.Builder protoBuilder = FileDescriptorProto.newBuilder()
        .setName("test1.proto");
    protoBuilder.addEnumTypeBuilder()
        .setName("Enum1").addValueBuilder().setName("V1").setNumber(1);
    protoBuilder.addMessageTypeBuilder()
        .setName("Message1").addFieldBuilder().setName("field1").setTypeName("Enum1").setNumber(999)
        .setLabel(Label.LABEL_REPEATED).getOptionsBuilder().setPacked(true);

    // The buildFrom() will fail, but it shouldn't!
    final FileDescriptor fileDescriptor = FileDescriptor.buildFrom(protoBuilder.build(), new FileDescriptor[0]);
  }

or just remove (expected = NullPointerException.class) and see it fail!

protobufel · 2014-09-17T00:42:40Z

BTW, you're not seeing a lot of problems because the protoc does a lot of cross-linking/name resolutions in protos for you already, so it gives you nice RESOLVED ones, and a lot of the Java "build" stuff, therefore, goes untested and problems are undetected!

Before, every charAt would emit (on android): ``` 0x00002104 adrp x17, #+0x1000 (addr 0x3000) 0x00002108 ldr w17, [x17, #20] 0x0000210c ldr x0, [x0, #128] 0x00002110 ldr x0, [x0, #328] 0x00002114 ldr lr, [x0, #24] 0x00002118 blr lr <-- Call into String.charAt(int) ``` Now, it emits the inlined implementation of charAt (branch is for possibly compressed strings): ``` 0x000020b4 ldur w16, [x4, #-8] 0x000020b8 tbnz w16, #0, #+0xc (addr 0x20c4) 0x000020bc ldrb w4, [x4, x0] 0x000020c0 b #+0x8 (addr 0x20c8) 0x000020c4 ldrh w4, [x4, x0, lsl #1] ``` PiperOrigin-RevId: 590821750

Before, every charAt would emit (on android): ``` 0x00002104 adrp x17, #+0x1000 (addr 0x3000) 0x00002108 ldr w17, [x17, #20] 0x0000210c ldr x0, [x0, #128] 0x00002110 ldr x0, [x0, #328] 0x00002114 ldr lr, [x0, #24] 0x00002118 blr lr <-- Call into String.charAt(int) ``` Now, it emits the inlined implementation of charAt (branch is for possibly compressed strings): ``` 0x000020b4 ldur w16, [x4, #-8] 0x000020b8 tbnz w16, #0, #+0xc (addr 0x20c4) 0x000020bc ldrb w4, [x4, x0] 0x000020c0 b #+0x8 (addr 0x20c8) 0x000020c4 ldrh w4, [x4, x0, lsl #1] ``` PiperOrigin-RevId: 591147406

…um value is detected. This should never happen, so I don't think it matters much exactly what kind of exception we throw. We could even arguably return null, but this option saves a lot of space while still preserving some error checking. See https://godbolt.org/z/jKhcKs3x1 for code gen. This generates much tighter bytecode and ARM assembly than alternatives. As this code is generated many times over, small wins in code size here can reduce icache pressure, APK size, and OAT size. This java code: ```java Object uoe() { throw new UnsupportedOperationException(); } Object npe2() { throw null; } ``` Generates this dex code: ``` .method uoe()Ljava/lang/Object; new-instance v0, Ljava/lang/UnsupportedOperationException; invoke-direct {v0}, Ljava/lang/UnsupportedOperationException;-><init>()V throw v0 .end method .method npe2()Ljava/lang/Object; const/4 v0, 0x0 throw v0 .end method ``` Which generates this OAT code: ``` java.lang.Object SomeProto.uoe() [84 bytes] 0x000081c0 sub x16, sp, #0x2000 (8192) 0x000081c4 ldr wzr, [x16] StackMap[0] native_pc=0x41c8, dex_pc=0x0, register_mask=0x0, stack_mask=0b 0x000081c8 str x0, [sp, #-48]! 0x000081cc str x22, [sp, #24] 0x000081d0 stp x23, lr, [sp, #32] 0x000081d4 ldr x21, [x21] StackMap[1] native_pc=0x41d8, dex_pc=0x0, register_mask=0x2, stack_mask=0b 0x000081d8 mov x22, x1 0x000081dc adrp x0, #+0x4000 (addr 0x0000c000) 0x000081e0 ldr w0, [x0, #4] 0x000081e4 ldr lr, [tr, #464] ; pAllocObjectInitialized 0x000081e8 blr lr StackMap[2] native_pc=0x41ec, dex_pc=0x0, register_mask=0x400000, stack_mask=0b 0x000081ec dmb ishst 0x000081f0 mov x1, x0 0x000081f4 mov x23, x1 0x000081f8 adrp x0, #+0x4000 (addr 0x0000c000) 0x000081fc ldr w0, [x0, #12] 0x00008200 ldr lr, [x0, #24] 0x00008204 blr lr StackMap[3] native_pc=0x4208, dex_pc=0x2, register_mask=0xc00000, stack_mask=0b 0x00008208 mov x0, x23 0x0000820c ldr lr, [tr, #1264] ; pDeliverException 0x00008210 blr lr StackMap[4] native_pc=0x4214, dex_pc=0x5, register_mask=0xc00000, stack_mask=0b java.lang.Object SomeProto.npe2() [36 bytes] 0x000080d0 sub x16, sp, #0x2000 (8192) 0x000080d4 ldr wzr, [x16] StackMap[0] native_pc=0x40d8, dex_pc=0x0, register_mask=0x0, stack_mask=0b 0x000080d8 str x0, [sp, #-32]! 0x000080dc stp x22, lr, [sp, #16] 0x000080e0 ldr x21, [x21] StackMap[1] native_pc=0x40e4, dex_pc=0x0, register_mask=0x2, stack_mask=0b 0x000080e4 mov x22, x1 0x000080e8 mov w0, #0x0 0x000080ec ldr lr, [tr, #1264] ; pDeliverException 0x000080f0 blr lr StackMap[2] native_pc=0x40f4, dex_pc=0x1, register_mask=0x400000, stack_mask=0b ``` This saves 84-36 = 48 bytes of OAT per method. PiperOrigin-RevId: 684258075

…um value is detected. This should never happen, so I don't think it matters much exactly what kind of exception we throw. We could even arguably return null, but this option saves a lot of space while still preserving some error checking. See https://godbolt.org/z/jKhcKs3x1 for code gen. This generates much tighter bytecode and ARM assembly than alternatives. As this code is generated many times over, small wins in code size here can reduce icache pressure, APK size, and OAT size. This java code: ```java Object uoe() { throw new UnsupportedOperationException(); } Object npe2() { throw null; } ``` Generates this dex code: ``` .method uoe()Ljava/lang/Object; new-instance v0, Ljava/lang/UnsupportedOperationException; invoke-direct {v0}, Ljava/lang/UnsupportedOperationException;-><init>()V throw v0 .end method .method npe2()Ljava/lang/Object; const/4 v0, 0x0 throw v0 .end method ``` Which generates this OAT code: ``` java.lang.Object SomeProto.uoe() [84 bytes] 0x000081c0 sub x16, sp, #0x2000 (8192) 0x000081c4 ldr wzr, [x16] StackMap[0] native_pc=0x41c8, dex_pc=0x0, register_mask=0x0, stack_mask=0b 0x000081c8 str x0, [sp, #-48]! 0x000081cc str x22, [sp, #24] 0x000081d0 stp x23, lr, [sp, #32] 0x000081d4 ldr x21, [x21] StackMap[1] native_pc=0x41d8, dex_pc=0x0, register_mask=0x2, stack_mask=0b 0x000081d8 mov x22, x1 0x000081dc adrp x0, #+0x4000 (addr 0x0000c000) 0x000081e0 ldr w0, [x0, #4] 0x000081e4 ldr lr, [tr, #464] ; pAllocObjectInitialized 0x000081e8 blr lr StackMap[2] native_pc=0x41ec, dex_pc=0x0, register_mask=0x400000, stack_mask=0b 0x000081ec dmb ishst 0x000081f0 mov x1, x0 0x000081f4 mov x23, x1 0x000081f8 adrp x0, #+0x4000 (addr 0x0000c000) 0x000081fc ldr w0, [x0, #12] 0x00008200 ldr lr, [x0, #24] 0x00008204 blr lr StackMap[3] native_pc=0x4208, dex_pc=0x2, register_mask=0xc00000, stack_mask=0b 0x00008208 mov x0, x23 0x0000820c ldr lr, [tr, #1264] ; pDeliverException 0x00008210 blr lr StackMap[4] native_pc=0x4214, dex_pc=0x5, register_mask=0xc00000, stack_mask=0b java.lang.Object SomeProto.npe2() [36 bytes] 0x000080d0 sub x16, sp, #0x2000 (8192) 0x000080d4 ldr wzr, [x16] StackMap[0] native_pc=0x40d8, dex_pc=0x0, register_mask=0x0, stack_mask=0b 0x000080d8 str x0, [sp, #-32]! 0x000080dc stp x22, lr, [sp, #16] 0x000080e0 ldr x21, [x21] StackMap[1] native_pc=0x40e4, dex_pc=0x0, register_mask=0x2, stack_mask=0b 0x000080e4 mov x22, x1 0x000080e8 mov w0, #0x0 0x000080ec ldr lr, [tr, #1264] ; pDeliverException 0x000080f0 blr lr StackMap[2] native_pc=0x40f4, dex_pc=0x1, register_mask=0x400000, stack_mask=0b ``` This saves 84-36 = 48 bytes of OAT per method. PiperOrigin-RevId: 684620833

xfxyjwf added this to the Release 2.6.1 milestone Sep 18, 2014

xfxyjwf mentioned this issue Oct 2, 2014

Fix descriptor validation logic for packed enum fields. #39

Merged

xfxyjwf closed this as completed in #39 Oct 3, 2014

jakiechris mentioned this issue Nov 20, 2017

[solved, pls close this issue] protobuf crashes on android 4.4.2 on Huawei p7 #3922

Closed

gatkinso mentioned this issue Nov 23, 2017

[C++] Intermittent Segmentation fault Mutex MacOS #2943

Closed

catskul mentioned this issue Aug 21, 2018

Problems with Python Layer: Protobuf Internal Error - Mutex Lock #3671

Closed

arnow117 mentioned this issue Apr 1, 2019

python: SIGSEGV when use PyImport_Import import symbol_database #5979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE #24

Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE #24

protobufel commented Sep 16, 2014

protobufel commented Sep 17, 2014

protobufel commented Sep 17, 2014

Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE #24

Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE #24

Comments

protobufel commented Sep 16, 2014

protobufel commented Sep 17, 2014

protobufel commented Sep 17, 2014