refactor protoparse, fixes multiple issues #316
Conversation
| // MaxTag is the maximum allowed tag number for a field. | ||
| MaxTag = 536870911 // 2^29 - 1 | ||
| // MaxNormalTag is the maximum allowed tag number for a field in a normal message. | ||
| MaxNormalTag = 536870911 // 2^29 - 1 |
There was a problem hiding this comment.
Nit: I might switch this from MaxNormalTag/MaxTag to MaxTag/MaxTagMessageSetWireFormat since the message set max tag is the edge case, but more a matter of preference
There was a problem hiding this comment.
I wanted a name like MaxTag to be the actual maximum value -- the greater of the two -- because I thought that would be more clear.
Maybe MaxNormalTag, MaxMessageSetTag, and MaxTag = MaxMessageSetTag?
There was a problem hiding this comment.
Yea that works! Or keep as is, either way :-)
There was a problem hiding this comment.
In addition to adding GetMaxTag(messageSet bool), I also left these other constants, but tweaked them per discussion above.
jhump
force-pushed
the
jh/fix-messageset-tag-range
branch
from
9e5baef
to
a643a41
Compare
- defers a lot of validation to after the AST is fully constructed - in particular, defers tag validation to after message options are parsed in order to know if it has message_set_wire_format option, which impacts allowed tag range - fixes issues with very large constant numbers (that overflow uint64 or underflow int64) - refactors grammar around option names and fixes issue where extensions on message options aren't parsed correctly - fixes issues related to groups in oneofs - fixes issues with reserved name validation - adds some util methods to shrink the boiler-plate for error creation and error handling - breaks up monolithic parser.go into three files: parser.go, validation.go and descriptor_protos.go - adds numerous new validation test cases to catch various issues that were fixed
jhump
force-pushed
the
jh/fix-messageset-tag-range
branch
from
a643a41
to
31226ba
Compare
desc/builder/field.go
Outdated
| @@ -508,6 +508,11 @@ func (flb *FieldBuilder) buildProto(path []int32, sourceInfo *dpb.SourceCodeInfo | |||
| def = proto.String(flb.Default) | |||
| } | |||
|
|
|||
| // normal (non-messageset) messages have lower tag limit, so enforce that here | |||
| if !isMessageSet && flb.number > internal.MaxNormalTag { | |||
There was a problem hiding this comment.
Do you want to enforce the isMessageSet case as well here? I'm sure you've thought of this and there's likely a reason you're not, but just checking.
There was a problem hiding this comment.
The code already validates that tags are never > internal.MaxTag when fields are created. This is a deferred check just for the tighter range imposed on non-messageset fields, which can't be performed until we know the field's context (e.g. is it part of a messageset or not).
There was a problem hiding this comment.
I changed this to just use internal.GetMaxTag so it's simpler to follow along
desc/protoparse/ast.go
Outdated
| @@ -729,17 +761,46 @@ type extensionRangeNode struct { | |||
| type rangeNode struct { | |||
| basicCompositeNode | |||
| stNode, enNode node | |||
| st, en int32 | |||
| enMax bool | |||
There was a problem hiding this comment.
Yeah, I'm not sure why I used st and en instead of start and end. I can do a quick rename for this and the two fields above.
desc/protoparse/validate.go
Outdated
| dpb "github.com/golang/protobuf/protoc-gen-go/descriptor" | ||
| ) | ||
|
|
||
| func basicValidate(res *parseResult, containsErrors bool) { |
There was a problem hiding this comment.
Sure... this was already called this FWIW (just moved from parser.go into its own file).
There was a problem hiding this comment.
Ah yea, I was going to add the caveat that this might already have been there - people commenting on something that was just moved around as part of a refactoring PR is a pet peeve of mine, so feel free to ignore this :-)
desc/protoprint/print.go
Outdated
| @@ -716,6 +716,10 @@ func (p *Printer) printMessageBody(md *desc.MessageDescriptor, mf *dynamic.Messa | |||
| } | |||
|
|
|||
| skip := map[interface{}]bool{} | |||
| maxTag := int32(internal.MaxNormalTag) | |||
There was a problem hiding this comment.
Super nit: You might want to turn this into a shared helper function GetMaxTag(isMessageSet bool) instead of having the public consts
|
Travis has been failing to report build status quite a lot lately :( Build is good: https://travis-ci.org/github/jhump/protoreflect/builds/679896778 Here goes nothing' |
message_set_wire_format option, which impacts allowed tag rangeuint64or underflowint64)errorWithPos(...),(*errorHandler) handleErrWithPos(...))parser.gointo three files:parser.go,validation.go, anddescriptor_protos.goFixes #275, #307, #308, #312, #313, #314, #315