From 5f75365f445e3de325ad0db81c5d9084f339cc6e Mon Sep 17 00:00:00 2001
From: wm4 <wm4@nowhere>
Date: Thu, 3 Oct 2019 00:41:30 +0200
Subject: [PATCH] vf_vapoursynth: fix crashing memory management mistake

As pointed out by @olifre in #7016, this line of code was wrong. p->opts
at this point is a struct allocated and managed by m_config. opts->file
is a string, and m_config explicitly frees it on destruction. The line
of code in question replaced the opts->file value, and made both the old
and new value children of the talloc allocation, so they were _also_
freed on destruction.

This crashed due to a double-free. First, talloc auto-freeing freed the
string, then m_config explicitly called talloc_free() on the stale
pointer again.

As @v-fox pointed out, commit 36dd2348a1e1 seems to have triggered the
crash. I suspect this code merely worked out of coincidence, since it
allowed m_config to free the value first. This removed it from the
auto-free list, and thus did not result in a double-free. The change
in order of calling alloc destructors changed the order of these calls.

There is no strong reason why new behavior (as introduced by commit
36dd2348a1e1) would be wrong (it feels like cleaner behavior). On the
other hand, what the vf_vapoursynth code did is clearly unclean and
going by the m_config API, you're not at all supposed to do this.
m_config manages all memroy referenced by option structs, the end.

@olifre's suggested fix also would have been correct (not just hiding
the issue), I prefer my fix, as it doesn't mess with the option struct
in tricky ways.

This wouldn't have happened if mpv were written in Haskell.
---
 video/filter/vf_vapoursynth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/video/filter/vf_vapoursynth.c b/video/filter/vf_vapoursynth.c
index 9edcf36792..d6cef08b3f 100644
--- a/video/filter/vf_vapoursynth.c
+++ b/video/filter/vf_vapoursynth.c
@@ -54,6 +54,7 @@ struct vapoursynth_opts {
 struct priv {
     struct mp_log *log;
     struct vapoursynth_opts *opts;
+    char *script_path;
 
     VSCore *vscore;
     const VSAPI *vsapi;
@@ -758,8 +759,7 @@ static struct mp_filter *vf_vapoursynth_create(struct mp_filter *parent,
         MP_FATAL(p, "'file' parameter must be set.\n");
         goto error;
     }
-    talloc_steal(p, p->opts->file);
-    p->opts->file = mp_get_user_path(p, f->global, p->opts->file);
+    p->script_path = mp_get_user_path(p, f->global, p->opts->file);
 
     p->max_requests = p->opts->maxrequests;
     if (p->max_requests < 0)
@@ -841,7 +841,7 @@ static int drv_vss_load(struct priv *p, VSMap *vars)
 {
     vsscript_setVariable(p->se, vars);
 
-    if (vsscript_evaluateFile(&p->se, p->opts->file, 0)) {
+    if (vsscript_evaluateFile(&p->se, p->script_path, 0)) {
         MP_FATAL(p, "Script evaluation failed:\n%s\n", vsscript_getError(p->se));
         return -1;
     }