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

map.js triggers assertion when encountering an integer-keyed map with key 0 #27

Open
5argon opened this issue Jul 15, 2020 · 2 comments
Labels
bug Something isn't working help wanted Extra attention is needed javascript port-fix triaged Issue has been triaged

Comments

@5argon
Copy link

5argon commented Jul 15, 2020

Version: 3.12.2
Language: Javascript

Following is a base64 string of bytes that has been coded from C# side. I could immediately decode at C# side just fine, but it failed to decode at JS library.

CiRlZjlhNTJlNC0wZDRmLTQyNzEtOGQzYi02ODZjYWE3N2M4ZWUSClBMQVlFUjc2MTYiCUxZWERQQkFDRkocaEpRQnFhT0t3SmZVbWtyTzF5UjZ5WVBXSWMxM1oCEgBaBggBEgIIAbIBAggBwgEMCIaVuPgFEOjNpLcB

It is coming from this proto definition. Please ignore references to other unknown messages, because the problem is on just field index 11.

message PlayerData {

    option (private_message) = true;
    reserved 3,5,7,8, 13 to 16;

    string player_id = 1;
    string display_name = 2;
    string short_player_id = 4;
    string start_playing = 6;
    string firebase_uid = 9;

    map<string,MinigameProgress> minigame_progresses = 10;

    //Problematic field on JS side
    map<int32,CharacterConfiguration> character_configurations = 11;

    int32 chilli = 12;

    int32 match_count_two_players = 17;
    int32 selected_character_configuration = 18;

    repeated UnlockedSet unlocked_sets = 19;

    repeated PermanentEvent permanent_events = 20; 

    map<string, google.protobuf.Timestamp> trial_timestamps = 21;

    SavedConfigurations saved_configurations = 22;

    repeated PurchasableFeatureId purchasable_features = 23;

    google.protobuf.Timestamp start_playing_20 = 24;
}

message CharacterConfiguration{

    E7.DuelOtters.Core.CharacterKind kind = 1;
    string headgear_system_name = 2;
    string clothing_system_name = 3;
    int32 winning_dance_id = 4;
    ColorMutation color_mutation = 5;
    EyeMutation eye_mutation = 6;
    CheekMutation cheek_mutation = 7;

    ClothingKind clothing_kind = 8;
    HeadgearKind headgear_kind = 9;
}

This is a ToString of real data coming from C#. Many were default, so it seems they had been efficiently stripped off.

{ "playerId": "ef9a52e4-0d4f-4271-8d3b-686caa77c8ee", "displayName": "PLAYER7616", "shortPlayerId": "LYXDPBACF", "firebaseUid": "hJQBqaOKwJfUmkrO1yR6yYPWIc13", "characterConfigurations": { "0": { }, "1": { "kind": "CharacterKind_Bomberjack" } }, "savedConfigurations": { "autoGameCenter": true }, "startPlaying20": "2020-07-14T19:41:58.384378600Z" }

When trying to deserialize in JS :

const b64 = "CiRlZjlhNTJlNC0wZDRmLTQyNzEtOGQzYi02ODZjYWE3N2M4ZWUSClBMQVlFUjc2MTYiCUxZWERQQkFDRkocaEpRQnFhT0t3SmZVbWtyTzF5UjZ5WVBXSWMxM1oCEgBaBggBEgIIAbIBAggBwgEMCIaVuPgFEOjNpLcB"
const buf = Buffer.from(b64, "base64")
PlayerData.deserializeBinary(buf)

It produces assertion failure :

Error [AssertionError]: Assertion failed
    at new goog.asserts.AssertionError (/Users/Sargon/Documents/Unity Projects/Main Projects/DuelOtters/Firebase/functions/node_modules/google-protobuf/google-protobuf.js:81:876)
    at Object.goog.asserts.doAssertFailure_ (/Users/Sargon/Documents/Unity Projects/Main Projects/DuelOtters/Firebase/functions/node_modules/google-protobuf/google-protobuf.js:82:257)
    at Object.goog.asserts.assert [as assert] (/Users/Sargon/Documents/Unity Projects/Main Projects/DuelOtters/Firebase/functions/node_modules/google-protobuf/google-protobuf.js:83:83)
    at Function.jspb.Map.deserializeBinary (/Users/Sargon/Documents/Unity Projects/Main Projects/DuelOtters/Firebase/functions/node_modules/google-protobuf/google-protobuf.js:411:241)

When compared with original source code, this assertion should be line 507 in js/map.js, jspb.Map.deserializeBinary : goog.asserts.assert(key != undefined); The key and value is going to be used in the map.set that follows so two asserts are there.

The problematic part in the data is the field index 11, map<int32,CharacterConfiguration> character_configurations = 11;

It is an int mapped to an another message. In the bytes, it has 2 members, int 0 mapped to an empty message, and int 1 mapped to a message with a bit of string data. Again, this is what they are at C#'s ToString debug :

 "characterConfigurations": { "0": { }, "1": { "kind": "CharacterKind_Bomberjack" } }

By simply removing the 0 keyed entry with default message value, and add it back to be keyed with 2 instead :

 "characterConfigurations": { "1": { "kind": "CharacterKind_Bomberjack" },  "2": { } }

JS is able to deserialize without triggering the assertion error. It seems like there is something with integer key at number 0. (C# could deserialize 0-keyed map in the protobuf bytes just fine)

@5argon 5argon changed the title Assertion mistake in map.js when encountering a map with all-default object entry map.js triggers assertion when encountering a map with all-default object entry Jul 15, 2020
@5argon 5argon changed the title map.js triggers assertion when encountering a map with all-default object entry map.js triggers assertion when encountering a map with all-default value entry Jul 15, 2020
@5argon 5argon changed the title map.js triggers assertion when encountering a map with all-default value entry map.js triggers assertion when encountering a integer-keyed map with key 0 Jul 15, 2020
@5argon 5argon changed the title map.js triggers assertion when encountering a integer-keyed map with key 0 map.js triggers assertion when encountering an integer-keyed map with key 0 Jul 15, 2020
@perezd perezd added javascript help wanted Extra attention is needed labels Oct 22, 2020
@perezd
Copy link

perezd commented Oct 22, 2020

Happy to accept a PR to address this!

@acozzette acozzette transferred this issue from protocolbuffers/protobuf May 16, 2022
@dibenede dibenede added bug Something isn't working triaged Issue has been triaged labels Sep 9, 2022
@dibenede
Copy link
Contributor

dibenede commented Sep 9, 2022

This seems like an intersection of the map and proto3 spec that we don't handle. We need to substitute in the default value in case the other end is proto3 and didn't serialize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed javascript port-fix triaged Issue has been triaged
Projects
None yet
Development

No branches or pull requests

3 participants