clugg / sm-json

A pure SourcePawn JSON encoder/decoder.
GNU General Public License v3.0
82 stars 8 forks source link

Fix `DeepCopy` problems for objects with ordered keys to prevent snapshot leaks on copy #35

Closed nickdnk closed 1 year ago

nickdnk commented 1 year ago

Hello

This is a proof-of-concept of changes that need to be applied (or, a suggestion) in order to fix https://github.com/clugg/sm-json/issues/34.

Basically, there were two problems here:

  1. If you don't use ordered keys, you will leak TrieSnapshots when DeepCopying via forwards in other plugins.
  2. If you use ordered keys, Merge gets the objects wrong when deep copying, and you get invalid handle exceptions on cleanup.

I don't have a solution to preventing leaks if you don't use ordered keys, but at least in my case, I can control that on my end by enabling it on all objects I send via forwards, and receiving plugins can copy these objects without leaking, and those will then also have ordered keys.

I don't fully understand why the merge logic doesn't work with ordered keys. I experienced it setting deep-copied properties (objects/arrays) multiple times on the same object, so you'd get objects like these (if you encode them out), which makes no sense:

{
    "player": {
        "user_id": 3,
        "steamid": "76561197996426755",
        "side": "ct",
        "name": "Nyxi",
        "is_bot": false
    },
    "friendly_fire": true,
    "player": {
        "user_id": 3,
        "steamid": "76561197996426755",
        "side": "ct",
        "name": "Nyxi",
        "is_bot": false
    }
}

This PR is just a draft to illustrate how the problem can be solved. I essentially just duplicate the logic from shallow copy in the deep copy code, but deep copying objects directly then, which prevents the use of the Merge function. It's also technically faster since you don't need to loop the objects twice.

The "real" solution is probably to figure out why Merge doesn't work with ordered keys, but I just couldn't. It makes no sense to me. I should add that shallow-copying does work with ordered keys, but since you re-iterate the same object again in a deep copy, it goes wrong the second time you attempt to set keys - when overriding the shallow copied objects with new objects.

nickdnk commented 1 year ago

Rebased on #33 just to keep myself sane. Still draft.

clugg commented 1 year ago

Thank you! Please see https://github.com/clugg/sm-json/issues/34#issuecomment-1483774832

clugg commented 1 year ago

The issue with ordered keys/iteration problems have been fixed in v4.1.3. The issue with leaking snapshots has been fixed in v5.0.0. Thanks again for your time and effort :)