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

Safer encode/decode variant. #27485

Merged
merged 3 commits into from Apr 1, 2019

Conversation

@Faless
Copy link
Contributor

commented Mar 28, 2019

This PR supersedes the other three. Closes #27399 Closes #27398 Closes #27442 .
Rationale.
Encoding decoding object variant is dangerous, and should never be the default.
The first 3 commits (e61a074, fbe7730, and 58a1f4a) do not change the exposed API but actively disable object variant encoding/decoding for all the methods that do not already expose an option to let the user decide (potentially breaking for users that relied on the dangerous functionality of object encoding).
It also swap the default parameters for encode_variant and decode_variant (not exposed directly) to make future internal code that uses them less likely to be bogus by mistake.

The other 2 commits (c06dd51, d9be3bf) add the full_objects/allow_objects parameters to respectively encoding and decoding functions:

  • Network peers get_var/put_var
  • File get_var/store_var
  • GDScript/Mono/VisualScript/Expression bytes2var/var2bytes base64_to_variant/variant_to_base64
  • Add MultiplayerAPI.allow_object_decoding member which deprecates PacketPeer.allow_object_decoding.

It also add docs for thos functions, but breaks ABI compatibaility (API compatibility for GDNative).

The question on what to do when cherry picking is left to release maintainers (@hpvb @akien-mga)... Should we only cherry-pick the first 3 (actively disabling a dangerous behaviour some users may rely on), or cherry pick all 5 commits, breaking ABI, still changing API behaviour for those dangerous cases, but giving users a chance to still decode objects by adding a parameter to their function call?

Please bear with me, helping out double-checking that changes are correct.

EDIT: Note that there is also a compat braking change where Expression now requires two parameters for bytes2var and var2bytes. I haven't found a way to have defaults there. Is it supported at all?

Fixes #27395

Use same boolean for objects encode and decode.
In a very unintuitive move encode needed false to encode an object,
decode needed true to decode it.
They now need the same value: `true`.

@Faless Faless added this to the 3.2 milestone Mar 28, 2019

@Faless Faless requested review from hpvb and neikeq Mar 28, 2019

@Faless Faless requested review from bojidar-bg, reduz, vnen and godotengine/documentation as code owners Mar 28, 2019

Show resolved Hide resolved modules/mono/glue/gd_glue.cpp
Show resolved Hide resolved modules/mono/glue/gd_glue.cpp
Show resolved Hide resolved modules/mono/glue/gd_glue.h
Show resolved Hide resolved modules/mono/glue/gd_glue.h
@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@neikeq thanks! Fixed. BTW, good job on the mono module, it's getting really awesome 👍 🥇

@Xrayez

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

This seems to work fine, but I wasn't dealing with variant encoding/decoding in the first place in my project, so at least no regressions hopefully. Integrate-tested file system encoding/decoding only.

Changes applied

Show resolved Hide resolved doc/classes/PacketPeer.xml Outdated

Faless added some commits Mar 28, 2019

Multiplayer API now respects allow_object_decoding
Add doc about allow_object_decoding in PacketPeer
Add object encoding param to serialization methods
Network peers get_var/put_var
File get_var/store_var
GDScript/Mono/VisualScript bytes2var/var2bytes
Add MultiplayerAPI.allow_object_decoding member which deprecates PacketPeer.allow_object_decoding.

Break ABI compatibaility (API compatibility for GDNative).

@Faless Faless force-pushed the Faless:io/encode_decode_safety_pr branch from 36e690b to 393e62b Apr 1, 2019

@akien-mga akien-mga merged commit e3bd84f into godotengine:master Apr 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

393e62b caused #27651, which #28052 aims to fix. The rest of the changes should likely also be reviewed to see if there are other cases where we do want to decode objects.

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

I was actually surprised to see that the Project Settings InputMap was working.
But yeah, I didn't export it, so of course it was working, it was using var2str and not var2bytes -.- 😿

@hpvb

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Cherry-picked for 3.1.1.

Entirely, breaking GDNative ABI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.