[Bug] JSON objects passed to forwards then iterated on causes memory problems

[nickdnk]

Description

If you have two plugins where one creates an object and forwards it to another, say:

Plugin A:

public void OnPluginStart()
{
  gForward = new GlobalForward("FORWARD_NAME", ET_Ignore, Param_Cell);

  JSON_Object obj = new JSON_Object();
  Call_StartForward(gForward);
  Call_PushCell(obj);
  Call_Finish();

  json_cleanup_and_delete(obj);
}

Consumed by another plugin like this:

Plugin B:

public void FORWARD_NAME(const JSON_Object obj) {
  obj.Iterate(); // Do whatever, just call Iterate(), either directly
  // or via DeepCopy() or ShallowCopy():
  JSON_Object copy = obj.DeepCopy();
  // Do something, maybe async (SQL for instance), with copy
  json_cleanup_and_delete(copy);
}

The fact that Iterate() calls delete on the cached snapshot that belongs to obj (this silently fails) and creates a new one (which will fail when Plugin A calls delete on it) causes the consuming plugin to leak the snapshot it creates every time this method is called. From the current code:

public int Iterate()
{
    if (! this.OrderedKeys) {
        if (this.Snap != null) {
            delete this.Snap; // This will attempt to delete the handle created by Plugin A,
            // if one exists - say if you had iterated on the object before passing it to the forward.
            this.Snap = null;
        }

        this.Snap = this.Snapshot(); // And this will create a new handle on Plugin A's object, which belongs to Plugin B.
        // Plugin A cannot clean this up in its `Cleanup`, and it will linger in Plugin B.
    }

    return this.Length;
}

Versions

sm-json: v4.x SourceMod Compiler: 1.11 SourceMod Build: Don't remember

Additional Information

I've been working on trying to solve this all day, with the help of @KillStr3aK on the SM Discord. I don't have a solution to this problem as long as Iterate is being used like it is. If the snapshot was returned and it was the callers responsibility to clean it up, like when normally iterating a string map, it would be solvable.

An issue was just opened on the AlliedModders GitHub (https://github.com/alliedmodders/sourcemod/issues/1957), but that is more generic and about the lack of an exception when accessing handles you shouldn't. If there is no solution from SourceMod's side, then sm-json will likely need to take a different approach to iterating objects.

[clugg]

Hey mate, thanks for raising this - this definitely didn't cross my mind as a use-case when building iteration the way I did. I had two ideas in mind when approaching this:

• Create a new iterator object, use it to allow multiple living snapshots for one object. This avoids the issues with cross-plugin memory but does require the user to cleanup the iterator once done.
• Make ordered keys the default behaviour. This removes all remaining need for snapshots, makes iterating rely fully on already known state and simplifies the code more generally. I also don't really see any cons to making everyone use ordered keys - there's no functional impact, output can only be considered nicer as a result and there's theoretically only a negligible performance hit for key tracking.

As such I've gone with option 2. These changes (plus other bugfixes you've mentioned in other issues/PRs) are now available on the develop branch (do note, this is still a WIP, I'm hoping to remove the 'ordered keys' tracking for arrays since it's redundant, but currently a lot of code relies on it). I've updated & confirmed the tests pass per these changes but haven't tested any further - if you could give it a go and let me know what you think that'd be much appreciated.

For reference, I believe the bug I mentioned/fixed here might also resolve the issue with deep copying/merging.

[nickdnk]

Hello

I'm just going to reply once here instead of all over the place:

1. I agree that a boolean for deep concat is unnecessary. Your proposal makes more sense.
2. I agree that ordered keys should be default and not something you turn off. Thank God we can get rid of the snapshots. You have no idea how much time I spent trying to figure out how this went wrong. It was especially difficult because of the inheritance structure (calls to .Super and .Meta), but I'm glad we got there.
3. I looked at the dev branch and it all looks good to me. Maybe you should consider adding the test I added for deep concat (https://github.com/clugg/sm-json/pull/33/files#diff-03055077dc1e1828ff248b40c45a8c45fc1390fea88e5a09f15aab164b617795R752), but just using DeeopCopy() on the object instead of the boolean I added.

I'll update Get5 to use v5 once you get it out. The next version of Get5 will likely be the last though, as Source 2 means the end of SourcePawn for CSGO/CS2 (as far as I understand at this stage), so we'll be starting from scratch rebuilding Get5 in vbscript or LUA or whatever they choose as the modding solution.

Thanks for fixing all the issues here.

[nickdnk]

Also, I just noticed that there is a "decode single quotes" option. Single-quoted JSON simply isn't valid JSON, so I don't really understand why it's there? If it introduces any kind of complexity for you, I would suggest you remove it. A new major version is a decent opportunity for this at least.

Unless of course this is because it supports JSON5, which would include comments, but I suspect not?

[clugg]

Thanks for the response, looks like we're on the same page here which is great.

I can appreciate the purpose of the test if we were going forward with Concat supporting deep copying internally, but since we're not and we already have tests for the two separate pieces of functionality (Concat and DeepCopy), I think it's unnecessary. Any issues introduced should be caught by one (or both) of those existing tests.

While single-quoted JSON isn't technically valid, it was requested as a feature in #10 and has very little impact on the code (only passed as a flag to json_is_string to decide whether to compare against ' or ", json_extract_string and json_extract_string_size simply take note of which quote they saw at the start of the buffer and use it to match the end quote). For such little maintainability cost I'm happy to keep it in as backwards compat for the few who may use it. You're correct that this library doesn't support JSON5.

Where did you hear about sourcemod/CS2 out of curiosity?

I've just pushed some more changes to avoid the ordered keys double-handling for arrays, which appears to work based on the tests. If you'd be able to give get5 a run against develop before I release v5 just to check nothing else pops up that'd be much appreciated :)

[nickdnk]

Alright on the test; I expect you have better overview of what's required to cover the code.

Single quote is up to you, just figured I'd mention it.

I will build get5's dev release against the current dev branch and have the guys play their "daily 5v5" on that version tonight (not that they'll have a clue about any of this, they just press the big play button and @ me when it doesn't work, but it gives me a chance to inspect if everything works). I'll let you know after!

[nickdnk]

And with regards to S2; it's the current narrative in the AlliedModders Discord that SourcePawn is dead as of CS2, but nothing is confirmed of course.

[nickdnk]

So an update after the games: I have no memory leaks or errors in Get5 with the current dev branch (8d72e43a55656d47fdcfcc526fe90c81d0382f5e). Everything seems to work as expected.

[clugg]

Thanks for testing that out for me! The fix has been released in v5.0.0 - thanks again for your help here.

[nickdnk]

Thanks for testing that out for me! The fix has been released in v5.0.0 - thanks again for your help here.

You're welcome. Thanks for fixing it. Now I can finally release the "completed" version of Get5 and have it be relevant for a grand total of ~3 months. /sadge.