vapoursynth / vapoursynth

A video processing framework with simplicity in mind
http://www.vapoursynth.com/
GNU Lesser General Public License v2.1
1.64k stars 151 forks source link

vsscript is not thread-safe. #390

Closed stuxcrystal closed 6 years ago

stuxcrystal commented 6 years ago

What happens?

The vapoursynth-module tracks its ScriptEnvironment data by its ID. If another filter tries to enter an environment these steps are executed:

  1. It checks if vsscript is enabled. If not, skip all further steps.
  2. It stores the current value of the _environment_id-variable onto the _environment_id_stack-list.
  3. It sets _environment_id to the new environment id.

To exit the environment afterwards these steps are followed:

  1. Store the last value stored on the _environment_id_stack-list to _environment_id

This is all well and good, until you consider that both variables are shared between all threads. This is especially problematic as filters like std.FrameEval and std.ModifyFrame call back to Python code from their respective worker thread.

Here is what might happen:

  1. The editor opens a new script-environment and executes a script there.
  2. The script uses a long running std.FrameEval-filter.
  3. The script ends and registers an output where the above filter is inside the filter-tree.
  4. The requests a frame triggering the FrameEval-callback.
  5. The user restarts the script.
  6. The editor opens a new script-environment and executes another script there.
  7. The old FrameEval-callback is still running and now tries to allocate a new filter. It will fail as the previous script-environment reset the environment.

The same thing will almost certainly happen with editors that support previewing multiple files at once.

How to reproduce?

I attached a zip-file with a proof of concept that causes VideoNode.set_output to attach itself to the wrong node (or worse, failing to create a filter). It consists of a module wrapping core vpy-functions (vsscript) and a script (confuse-environment.py) that reproduces the error. confuse-environment.zip

Why is this bad

VSScript is effectively unstable when more than one Environment is in use. This is traditionally the case in VapoursynthEditor. Especially since the core is managed by the Python Garbage Collector. If any module holds reference over the core, the core does not get freed.

How to fix.

Make both variables thread-local.

myrsloik commented 6 years ago

Interesting, I always assumed the GIL would make the operations kinda acceptable. Will review it tomorrow.