Skip to content

Regression in 3.12: toString() returns incorrect ascii when there are duplicate keys in a map #7505

Closed

Description

@thesamet

Prior to protobuf 3.12, when parsing a map using TextFormat:

  • If there are duplicate map keys, the last key seen is used.
  • toString emits the duplicate keys in the original order, so if the ascii representation was parsed again, the same last key will be used, and the original message will be preserved.

In 3.12.0-rc-1 and 3.12.0-rc-2:

  • The last key seen is used.
  • toString does not retain the original order. In some cases, the last value printed for a key, is not necessarily the value that is being used in a message. As a result, parsing again the output of toString results in a different message.

In other words, m.toString() is producing a text representation that when parsed does not give the original m back.

What version of protobuf and what language are you using?
Version: 3.12.0-rc-2
Language: Java
What operating system (Linux, Windows, ...) and version?
Ubuntu 19.10

What did you do?

np.proto:

syntax = "proto3";

package mypkg;

message Msg {
  map<int32, int32> m = 1;
}

Main.java:

import mypkg.Np.Msg;

import com.google.protobuf.TextFormat;
import com.google.protobuf.util.JsonFormat;

import java.lang.String;

public class Main {
    public static void main(String[] args) throws Exception {
            String input =
            "m: {\n" +
            "  key: 1\n" +
            "  value: 1\n" +
            "}\n" +
            "m: {\n" +
            "  key: -2147483647\n" +
            "  value: 5\n" +
            "}\n" +
            "m: {\n" +
            "  key: 1\n" +
            "  value: -1\n" +
            "}\n";

            Msg p = TextFormat.parse(input, Msg.class);
            int i1 = p.getMMap().get(1);
            Msg p2 = TextFormat.parse(p.toString(), Msg.class);
            int i2 = p2.getMMap().get(1);
            System.out.println(i1);  // prints -1
            System.out.println(i2);  // prints 1, expected -1
    }
}

What did you expect to see
Either a failure to parse, or:

-1
-1

What did you see instead?

1
-1

Activity

  1. haberman commented on May 14, 2020

    @haberman
    Member

    What is the output of the p.toString() call here?

  2. haberman commented on May 14, 2020

    @haberman
    Member

    I was able to reproduce, I see the output:

    int32_to_int32_field {
      key: 1
      value: -1
    }
    int32_to_int32_field {
      key: -2147483647
      value: 5
    }
    int32_to_int32_field {
      key: 1
      value: 1
    }
    
  3. haberman commented on May 14, 2020

    @haberman
    Member

    I think the subtraction here is overflowing:
    https://github.com/protocolbuffers/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/TextFormat.java#L513

    If I change this to use Integer.compare((int) getKey(), (int) b.getKey()) your test passes.

    Thanks for the report. I'll make sure this is fixed before 3.12.0 is released.

  4. haberman commented on May 16, 2020

    @haberman
    Member

    The fix is released in 3.12.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions