3.9+ C# lack of backward compatibility with extensions

[thesoftwarejedi]

Version: v3.10
Language: C#
OS: Windows/Linux
Runtime: .NET Core 2.1

What did you do?
Steps to reproduce the behavior:

1. Generate and compile a proto which has extensions in it to C# using version 3.8 or earlier, or use Grpc.Tools (which is submoduled to 3.x of this repo).
2. Use the new DLL in a C# project which uses a reference to 3.9 or later of Google.Protobuf. Note again, this can actually be done from the same project if you use Grpc.Tools to compile, and Google.Protobuf v3.9+ as a reference.
3. Note exception when running and any code that will trigger the static initializer on the generated class(es).

What did you expect to see
No exception

What did you see instead?
A static initializer error reading the descriptor.

Index was outside the bounds of the array.
   at Google.Protobuf.Reflection.ExtensionCollection.<>c__DisplayClass2_0.<.ctor>b__0(FieldDescriptorProto extension, Int32 i) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\ExtensionCollection.cs:line 51
   at Google.Protobuf.Reflection.DescriptorUtil.ConvertAndMakeReadOnly[TInput,TOutput](IList`1 input, IndexedConverter`2 converter) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\DescriptorUtil.cs:line 59
   at Google.Protobuf.Reflection.ExtensionCollection..ctor(FileDescriptor file, Extension[] extensions) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\ExtensionCollection.cs:line 49
   at Google.Protobuf.Reflection.FileDescriptor..ctor(ByteString descriptorData, FileDescriptorProto proto, IEnumerable`1 dependencies, DescriptorPool pool, Boolean allowUnknownDependencies, GeneratedClrTypeInfo generatedCodeInfo) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\FileDescriptor.cs:line 87
   at Google.Protobuf.Reflection.FileDescriptor.BuildFrom(ByteString descriptorData, FileDescriptorProto proto, FileDescriptor[] dependencies, Boolean allowUnknownDependencies, GeneratedClrTypeInfo generatedCodeInfo) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\FileDescriptor.cs:line 344
   at Google.Protobuf.Reflection.FileDescriptor.FromGeneratedCode(Byte[] descriptorData, FileDescriptor[] dependencies, GeneratedClrTypeInfo generatedCodeInfo) in T:\src\github\protobuf\csharp\src\Google.Protobuf\Reflection\FileDescriptor.cs:line 385
   at redacted

Anything else we should know about your project / environment
This comes from here:
https://github.com/protocolbuffers/protobuf/blob/master/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs#L56 where it looks like closure over extensions makes the poor assumption that message.Proto.Extension will have the same length.

v3.9 added extensions into the generated code as seen here. However older code generated without this GeneratedClrType constructor overload fails at the static initializer reading the Descriptor as it isn't aware of the extensions.

This is also very related to #5957 which seems to indicate that this type of compatibility will be marked by an API version, referenced here.

Also opened grpc/grpc#20557 in the Grpc project to start a discussion about updating the version of protoc submoduled there.

/cc: @ObsidianMinor @jtattermusch

[clehene]

Spent the morning on this too.

@thesoftwarejedi it should be fine if we'd only downgrade c# libs, and leave backend on 3.10, right?

[jtattermusch]

Can you provide an example of your .proto file (or a minimal repro version of it)?
(also, is it a syntax = proto2 file? I don't think a proto3 file can define extensions).

[ObsidianMinor]

Yeah it was a mistake made when breaking up the original implementation into separate PRs (since the original implementation was made under the assumption that everything would be added in one fell swoop and different features wouldn't be left incomplete). I had opened a PR before the finale PR that made a temp fix for 3.9 and 3.10 but it never got merged so I assumed we were just going to leave it broken until 3.11 when extensions fully work. I'll write a fix today or this weekend.

Proto3 extensions can only be defined for the options types in descriptor.proto @jtattermusch.

syntax = "proto3";

import "google/protobuf/descriptor.proto";

extend google.protobuf.MessageOptions {
    int32 a = 999999;
}

Should trigger it if I'm correct. I don't think this got picked up by tests because internal access is given to the test project which means any generated extensions would compile while failing for everyone else. Plus the compatibility tests don't work so it's hard to test backwards compatibility...

[jtattermusch]

@ObsidianMinor can you come up with a fix?

[ObsidianMinor]

Sure @jtattermusch. I'll write it as 2 separate PRs, one will add length checks to the lib for master (so past code-gen works with updated versions of the lib) and the other will add proto2 checks to the code-gen for 3.10.x and 3.9.x if we want.

[thesoftwarejedi]

Spent the morning on this too.

@thesoftwarejedi it should be fine if we'd only downgrade c# libs, and leave backend on 3.10, right?

Not sure what you mean. You're fine if the usage of the protos that have extensions has a reference >= the version of protoc used to generate them.

[jtattermusch]

The patched nuget is now available: https://www.nuget.org/packages/Google.Protobuf/3.10.1
Can you please test and report back if it solves the issue.

[abe545]

@ObsidianMinor @jtattermusch - we are experiencing issues with 3.10.1 as well - what in 3.9 and 3.10 was an IndexOutOfBoundsException is a ArgumentNullException in 3.10.1:

Message: 
    System.TypeInitializationException : The type initializer for 'Google.Api.ServiceReflection' threw an exception.
    ---- System.ArgumentNullException : Value cannot be null. (Parameter 'extension')
  Stack Trace: 
    ServiceReflection.get_Descriptor()
    Service.get_Descriptor()
    IMessage.get_Descriptor()

If we use 3.8, everything still works. The only protos we don't generate ourselves with protoc are from Google.Api.CommonProtos 1.7.0.

[thesoftwarejedi]

Yeah I never confirmed this fix. Sorry that we haven't had a chance to verify this fix. Our project is not in a place to risk trying version moving around again.

[jtattermusch]

3.11 with the necessary fixes should be available very soon.