godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.93k stars 21.15k forks source link

rpc call inst2dict/dict2inst results in EncodedObjectAsID #42725

Closed MikeSchulze closed 1 year ago

MikeSchulze commented 4 years ago

Godot version: Godot Engine v3.2.3.stable.mono.official (c) 2007-2020

OS/device including version: Windows 10 x64

Issue description: When i serialize/deserialize an class with an Array as member and send it via rpc to the server the Array contains 'EncodedObjectAsID' instead of the original type rpc("receive_event", _connection_id, inst2dict(event))

Steps to reproduce:

remote func receive_event(id:int, obj): var event := dict2inst(obj) prints("received", event)


The Event class

class_name Event extends Resource

class InnerClass extends Resource: var _value:String = "bar"

var _name:String var _data:Array = Array()

func _init(): _name = "foo" _data = [InnerClass.new()]

func _to_string(): return "name:%s, data:%s" % [_name, _data]

the output for rpc call

send name:foo, data:[[Resource:1514]] received name:foo, data:[[EncodedObjectAsID:24197]]


But when i do same logic locally the deserialized object is correct

var serialized := inst2dict(obj) var deserialized := dict2inst(serialized ) prints( "serialized", serialized) prints("deserialized", dict2inst(serialized))

the output is 

serialized {@path:res://addons/gdUnit/src/core/event/Event.gd, @subpath:, _data:[[Resource:1514]], _name:foo} deserialized name:foo, data:[[Resource:1514]]



This issue blocks me to serialize/deserialize my data form client to server

**Minimal reproduction project:**
Xrayez commented 4 years ago

See documented limitation in PacketPeer.allow_object_decoding:

Warning: Deserialized objects can contain code which gets executed. Do not use this option if the serialized object comes from untrusted sources to avoid potential security threats such as remote code execution.

See also #27485.

Can you try to do the same thing by first enabling object decoding? Something like this:

extends Node2D

func _ready():
    var peer = NetworkedMultiplayerENet.new()
    var port = 12345
    peer.allow_object_decoding = true
    peer.create_server(port)
    get_tree().network_peer = peer

Not sure if will work, and allow_object_decoding is deprecated.

You should probably consider finding alternative ways to serialize your game data to be sent over network. Namely, you could try:

var json = to_json(inst2dict(obj))
var obj = dict2inst(parse_json(json))
MikeSchulze commented 4 years ago

Thank you for the security explanation

var json = to_json(inst2dict(obj)) var obj = dict2inst(parse_json(json))

Results into wrong deserialisation because json can't transport types. The original dictionary created by 'inst2dict(obj)' has different value types and when you parse back from json you have only STRING and FLOAT types. But for a correct deserialization, the 'dict2inst' func needs the rigth types.

So looks like Godot has no default way to serialize/encrypt and decrypt/deserialze functionallity ? Apart from the allow_object_decoding flag which is deprecated

Xrayez commented 4 years ago

So looks like Godot has no default way to serialize/encrypt and decrypt/deserialze functionallity ?

Perhaps you need to use var2str and str2var functions as well, which may provide a better support for serialization, see godotengine/godot-docs#3893. You'd still need to carefully think about serializing objects that way, which is likely better done with inst2dict.

That said, Godot doesn't seem to provide an out-of-the-box serialization solution, and I think this should be done on per-project basis anyways.

MikeSchulze commented 4 years ago

I tested now

var as_string= var2str (inst2dict(obj))
var obj = dict2inst(str2var(as_string))

But still the same issue the member _data is wrong deserialized, the type changed from EncodedObjectAsID to Resource. And is not fully deserialized back to InnerClass

That said, Godot doesn't seem to provide an out-of-the-box serialization solution, and I think this should be done on per-project basis anyways.

Looks like so ;( So i have to write my custom serde stuff

MikeSchulze commented 4 years ago

using var2str/str2var over RPC also not works


func send_event(event:Event):
    rpc("receive_event", _connection_id, var2str(event))

remote func receive_event(id:int, obj):
    var event = str2var(obj)
    prints("received", event)

the deserialize object is a kind of the original object but the functions not accessible.
I print out the 'get_method_list' and i can see the flags are different to the original
e.g.
`func foo()`
has oiginal the flags METHOD_FLAG_FROM_SCRIPT|METHOD_FLAGS_DEFAULT
but after deserialize the flag is changed to METHOD_FLAGS_DEFAULT

This ends up in a unusable object.
Is this the expected behavior?
Calinou commented 4 years ago

@MikeSchulze IIRC, objects can't be serialized, only primitive/value types can. See also Binary serialization API (but that's for var2bytes()).

MikeSchulze commented 4 years ago

@Calinou you wrote only primitive/value types can be serialized, but when i look in the implementation it supports Object/Resource/GdScript to serialize/deserialize

And it works when i do it localy var event2 = str2var(var2str(event)) The object is fully serialized and deserialized and useable. Only when send the serialized version over RPC than the deserialization ends up in an unuseable object.

jonbonazza commented 4 years ago

Same issue as #41917

Faless commented 3 years ago

The problem is that inst2dict does not recursively encode instances, so:

class C2:
    var c2 = 2

class C1:
    var c = 1
    var c2 = C2.new()

func _ready() -> void:
    print([inst2dict(C1.new())])

will produce:

[{@path:res://Control.gd, @subpath:C1, c:1, c2:[Reference:1219]}]

Since Reference is not converted, it is encoded as an ID unless object encoding is enabled.

So the question is, should inst2dict act recursively? should that be the default or an option?

Xrayez commented 3 years ago

So the question is, should inst2dict act recursively? should that be the default or an option?

33137 is there to add recursive option (disabled by default), feel free to salvage if needed.

Faless commented 3 years ago

Honestly, I think inst2dict is not the right way to do serialization anyway, because it's always going to be wasteful, and we should provide something like serialization annotations to script languages instead.

YuriSizov commented 1 year ago

So this is duplicate of https://github.com/godotengine/godot/issues/6533 then, and another case for a larger issue with recursive encoding.