Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proto: Marshal message with oneof field does not produce correct field order #395

Closed
zjb0807 opened this issue Jul 20, 2017 · 7 comments
Closed

Comments

@zjb0807
Copy link

zjb0807 commented Jul 20, 2017

proto file:

syntax = "proto3";
package main;

message Request {
	oneof value{
		RequestUserCreate userCreate = 1;
	}
	int64 uid = 2;
}

message RequestUserCreate {
	int64 userUid  = 1;
}

go source:

package main

import (
    "fmt"
    "encoding/hex"
    proto "github.com/golang/protobuf/proto"
)

func main() {
    req := &RequestUserCreate{}
    req.UserUid = 123456
    data, err := proto.Marshal(req)
    if err != nil {
        fmt.Println(err)
    }                                                                                                                                          
    fmt.Println(hex.EncodeToString(data))

    request := &Request{}

    request.Value = &Request_UserCreate{req}
    data, err = proto.Marshal(request)
    if err != nil {
        fmt.Println(err)
    }   
    fmt.Println(hex.EncodeToString(data))
    request.Uid = 1
    data, err = proto.Marshal(request)
    if err != nil {
        fmt.Println(err)
    }
    fmt.Println(hex.EncodeToString(data))
}      

the results:

08c0c407
0a0408c0c407
10010a0408c0c407

the uid tag is 2,why proto.Marshal the result is before userCreate ?

@cybrcodr
Copy link
Contributor

I'm not sure what you're asking about. I can explain the results based on https://developers.google.com/protocol-buffers/docs/encoding ...

RequestUserCreate{UserUid: 123456} == 08c0c407
08 ~ wire type of 0 (varint/int64) for field number 1 (userUid)
rest are for the value of 123456

Request{Value: &Request_UserCreate{req}} == 0a0408c0c407
where req is above value.
0a ~ wire type of 2 (embedded message) for field number 1 (userCreate)
04 ~ since it is an embedded message, 04 is the length varint of the embedded message, i.e. 4 bytes for 08c0c407.

Request{Value: &Request_UserCreate{req}, Uid: 1} == 10010a0408c0c407
10 ~ wire type of 0 (varint/int64) for field number 2 (uid)
01 ~ value of above field which is 0
rest is for field number 1 same as above.

Please explain more what you're asking about? Thanks.

@zjb0807
Copy link
Author

zjb0807 commented Jul 21, 2017

in Java,the result is:


08c0c407
0a0408c0c407
0a0408c0c4071001

In the order of size of tag, it should be 0a0408c0c4071001, but the result is not the same as expected

@cybrcodr
Copy link
Contributor

The order of the fields should not matter. You should be able to deserialize those bytes in Java or other language implementations regardless of the field order.

@zjb0807
Copy link
Author

zjb0807 commented Jul 21, 2017

Yes, i can deserialize those bytes in Java, but if i use the bytes to sign, the server check sign will be wrong. This may be a bug

@cybrcodr
Copy link
Contributor

I guess the following does mention about field orders ...
https://developers.google.com/protocol-buffers/docs/encoding#order

"While you can use field numbers in any order in a .proto, when a message is serialized its known fields should be written sequentially by field number, as in the provided C++, Java, and Python serialization code. This allows parsing code to use optimizations that rely on field numbers being in sequence. However, protocol buffer parsers must be able to parse fields in any order, as not all messages are created by simply serializing an object – for instance, it's sometimes useful to merge two messages by simply concatenating them."

I'd probably would not use the serialized bytes to do any signing though. If there are unknown fields, it states ...

"If a message has unknown fields, the current Java and C++ implementations write them in arbitrary order after the sequentially-ordered known fields."

@cybrcodr cybrcodr added the bug label Jul 21, 2017
@cybrcodr cybrcodr changed the title "oneof" type proto.Marshal the result is not the same as expected proto.Marshal message with oneof field does not produce correct field order Jul 21, 2017
@zjb0807
Copy link
Author

zjb0807 commented Jul 21, 2017

Thank you for your advice, The client and server versions are consistent, it will avoid using the unknown fields.

@dsnet dsnet changed the title proto.Marshal message with oneof field does not produce correct field order proto: Marshal message with oneof field does not produce correct field order Feb 14, 2018
@dsnet
Copy link
Member

dsnet commented Feb 14, 2018

I'm closing this for the following reasons:

  • The suggestion in the proto guide is for performance reasons on the parser. However, depending on the implementation of the serializer, keeping the tags sorted may not be performant either. Thus, this is not a strong argument why we must sort the order.
  • The original use case for this issue is for signing the encoded bytes. This requires some form of canonical encoding, which the protobuf specification does not provide. Even more important, proto.Marshal does not even guarantee that the encoding is deterministic.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants