search

foundryvtt / foundryvtt

Public issue tracking and documentation for Foundry Virtual Tabletop - software connecting RPG gamers in a shared multiplayer environment with an intuitive interface and powerful API.
https://foundryvtt.com/
198 stars 10 forks source link

Add a maximum depth guard to diffObject and deepClone which throws a more informative error #10324

Closed K4rakara closed 2 weeks ago

K4rakara commented 4 months ago

What happened?

The diffObject helper inside of commons.js doesn't appear to account for objects that are cyclic. This can cause infinite recursion, and slows down foundry significantly. See the following stacktrace: image

Granted, it's likely that this problematic input is being supplied by a third party module, but I'm able to reproduce the bug with a three line script.

What ways of accessing Foundry can you encounter this issue in?

Reproduction Steps

let cyclic = { foo: { } };
cyclic.foo.bar = cyclic;
diffObject(cyclic, cyclic);

What core version are you reporting this for?

Version 11 Release, Build 315

Relevant log output

Uncaught InternalError: too much recursion
    _difference http://localhost:30000/scripts/commons.js:2193
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
    _difference http://localhost:30000/scripts/commons.js:2210
    diffObject http://localhost:30000/scripts/commons.js:2227
    diffObject http://localhost:30000/scripts/commons.js:2219
debugger eval code:2193:34

Bug Checklist

K4rakara commented 4 months ago

deepClone also appears to have this issue, but I haven't encountered that in the wild personally.

aaclayton commented 2 weeks ago

We have some tension with these methods about whether to prefer performance or flexibility. These diff and clone methods are used intensively for some real-time operations where the performance overhead of creating a supplementary set for remembering which objects have been visited by the operation could be undesirable. I'll take a look and do a benchmark of the difference.