search

protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.75k stars 15.51k forks source link

[Golang] Marshal orders fields incorrectly #9112

Closed wkornewald closed 8 months ago

wkornewald commented 3 years ago

What version of protobuf and what language are you using? Version: v3.18.1 with Go lib v1.27.1 Language: Go

What operating system (Linux, Windows, ...) and version?

Linux

What runtime / compiler are you using (e.g., python version or gcc version)

Official Go lib v1.27.1 (https://github.com/protocolbuffers/protobuf-go)

What did you do? Steps to reproduce the behavior:

EDIT: I've open-sourced my code here: https://github.com/ensody/androidmanifest-changer You can run the app directly against a binary/protobuf AndroidManifest.xml file (it doesn't have to be an AAB or APK). See below for the zip file with the sample manifests.

My goal was to modify the protobuf-based AndroidManifest.xml within Android AAB files and also the aapt2 conversion of the manifest from APKs.

I took the protobufs from here:

The original and modified AndroidManifest files and their human-readable dumps can be found here: manifest.zip

The data in those files corresponds to the XmlNode type from Resources.proto.

In the original data all fields are ordered by number. When you simply unmarshal and marshal the data (even without any modifications) what you get is that the SourcePosition is suddenly always written before all other elements. So we lose the number-based field ordering. When you take an aab file with such a rewritten AndroidManifest.xml and inspect it in Android Studio you see some strange broken output. In contrast, if I manipulate the manifest with vtprotobuf and use MarshalVT() the result has correct field ordering and it can be correctly inspected in Android Studio.

For maximum compatibility with other libs it would be great if the Go protobuf lib could be fixed to always use number-based ordering.

wkornewald commented 3 years ago

Update: I've open-sourced my code, so you can easily reproduce it with a ready-made app: https://github.com/ensody/androidmanifest-changer

znkr commented 3 years ago

The order in which fields are serialized is left as an implementation detail. This allows for optimizations in the serialization path because the serializier doesn't need to worry about an encoding order but can just output data as it's stored in memory. We are not considering changing this behavior in the Go implementation. If this causes problems in AndroidStudio, it's likely a bug there.

https://developers.google.com/protocol-buffers/docs/encoding#order

github-actions[bot] commented 9 months ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

github-actions[bot] commented 8 months ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.