golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.74k stars 1.58k forks source link

encoding/prototext: parser should allow whitespace between minus sign and number in negative numeric literal #1526

Closed jhump closed 1 year ago

jhump commented 1 year ago

The text format specification explicitly states that whitespace and comments are allowed between the minus sign and the subsequent numeric component of a negative numeric literal. I can confirm that the C++ implementation correctly adheres to this. But the Go implementation in prototext does not.

Here's a trivial example.

// test.proto
syntax = "proto3";

message Foo {
  int32 id = 1;
}

Compile the above to Go via command like so:

protoc --go_out=paths=source_relative,Mtest.proto=foo.com/test/main:. test.proto

Here's the rest of the Go program

// main.go (in same directory as test.pb.go generated from above)
package main

import (
  "fmt"
  "google.golang.org/protobuf/encoding/prototext"
)

func main() {
  data := "id : - 23"
  var msg Foo
  err := prototext.Unmarshal([]byte(data), &msg)
  if err != nil {
    panic(err)
  }
  fmt.Println(&msg)
}

The above program panics with the following error:

proto: syntax error (line 1:6): invalid scalar value: -

I would instead expect this to echo back the message data with output like so:

id:-23
puellanivis commented 1 year ago

Unexpected standards esotericisms. This reminds me a whole lot about how technically you can embed comments into email addresses as well, except that no one actually supports that.

cybrcodr commented 1 year ago

Iirc, they didn't have the textproto spec publicized yet at the time this was written and no compliance test for it too... https://github.com/protocolbuffers/protobuf-go/blob/master/internal/encoding/text/decode_number.go#L79-L81

Here's a possible solution. parseNumber func needs to account for the extra whitespace and/or comments. It may also need to produce the bytes/string for properly setting Token.str field in parseNumberValue.

I currently don't have the bandwidth to work on this. If anyone has, feel free to send CL.

jhump commented 1 year ago

I currently don't have the bandwidth to work on this. If anyone has, feel free to send CL.

@cybrcodr, I got ya. https://go-review.googlesource.com/c/protobuf/+/473015 Thanks for the pointer!