js: fix tiny leaks if js_try throws(!)

As it turns out, js_try can throw if it runs out of try-stack (without/before entering either the try part or the catch part). If it happens, then C code which does allocation -> try will leak. In mpv there were two places which do alloc and then try, one of them as part of the autofree system. On both cases the leak is the smallest possible (zero allocation) - talloc_new(NULL); It's very unlikely to trigger - an autofree mpv API should be called when the try-stack is exactly full, and our next try will throw (and guaranteed to get caught at an outer level, but with a leak). Fix that by doing the allocation inside the try block, so that if try throws before it's entered then nothing got allocated/leaked. Mujs internal code also has/had similar leaks, which are getting fixed around this time (July 2021, post mujs 1.1.3). [1] exhaust the try-stack or call-stack, whichever comes first: function kaboom() { try { kaboom() } catch(e) {} }

Avi Halachmi (:avih) committed 5 years ago

Commit 5ed03381 parent 36b7cff1 file · +41 −16Browse files

Showing the changes to player/javascript.c (1 of 1 file). Show all

Unified · Split

player/javascript.c+41−16

 //   padded with undefined if called with less, or bigger if called with more.
 //
 // - Almost all vm APIs (js_*) may throw an error - a longjmp to the last
-//   recovery/catch point, which could skip releasing resources. Use protected
+//   recovery/catch point, which could skip releasing resources. This includes
-//   code (e.g. js_pcall) between aquisition and release. Alternatively, use
+//   js_try itself(!), except at the outer-most [1] js_try which is always
-//   the autofree mechanism to manage it more easily. See more details below.
+//   entering the try part (and the catch part if the try part throws).
+//   The assumption should be that anything can throw and needs careful setup.
+//   One such automated setup is the autofree mechanism. Details later.
 //
 // - Unless named s_foo, all the functions at this file (inc. init) which
 //   touch the vm may throw, but either cleanup resources regardless (mostly
 //   autofree) or leave allocated resources on caller-provided talloc context
 //   which the caller should release, typically with autofree (e.g. makenode).
 //
-// - Functions named s_foo (safe foo) never throw, return 0 on success, else 1.
+// - Functions named s_foo (safe foo) never throw if called at the outer-most
+//   try-levels, or, inside JS C functions - never throw after allocating.
+//   If they didn't throw then they return 0 on success, 1 on js-errors.
+//
+// [1] In practice the N outer-most (nested) tries are guaranteed to enter the
+//     try/carch code, where N is the mujs try-stack size (64 with mujs 1.1.3).
+//     But because we can't track try-level at (called-back) JS C functions,
+//     it's only guaranteed when we know we're near the outer-most try level.
 /**********************************************************************
  *  mpv scripting API error handling
 //   inserted into the vm using af_newcfunction, but otherwise used normally.
 //
 //  To wrap an autofree function af_TARGET in C:
-//  1. Create a wrapper s_TARGET which runs af_TARGET safely inside js_try.
+//  1. Create a wrapper s_TARGET which does this:
-//  2. Use s_TARGET like so (always autofree, and throws if af_TARGET threw):
+//      if (js_try(J))
-//       void *af = talloc_new(NULL);
+//          return 1;
-//       int r = s_TARGET(J, ..., af);  // use J, af where the callee expects.
+//      *af = talloc_new(NULL);
+//      af_TARGET(J, ..., *af);
+//      js_endtry(J);
+//      return 0;
+//  2. Use s_TARGET like so (frees if allocated, throws if {s,af}_TARGET threw):
+//       void *af = NULL;
+//       int r = s_TARGET(J, ..., &af);  // use J, af where the callee expects.
 //       talloc_free(af);
 //       if (r)
 //           js_throw(J);
+//
+//  The reason that the allocation happens inside try/catch is that js_try
+//  itself can throw (if it runs out of try-stack) and therefore the code
+//  inside the try part is not reached - but neither is the catch part(!),
+//  and instead it throws to the next outer catch - but before we've allocated
+//  anything, hence no leaks on such case. If js_try did get entered, then the
+//  allocation happened, and then if af_TARGET threw then s_TARGET will catch
+//  it (and return 1) and we'll free if afterwards.
 // add_af_file, add_af_dir, add_af_mpv_alloc take a valid FILE*/DIR*/char* value
 // respectively, and fclose/closedir/mpv_free it when the parent is freed.
 typedef void (*af_CFunction)(js_State*, void*);
 // safely run autofree js c function directly
-static int s_run_af_jsc(js_State *J, af_CFunction fn, void *af)
+static int s_run_af_jsc(js_State *J, af_CFunction fn, void **af)
 {
     if (js_try(J))
         return 1;
-    fn(J, af);
+    *af = talloc_new(NULL);
+    fn(J, *af);
     js_endtry(J);
     return 0;
 }
     af_CFunction fn = (af_CFunction)js_touserdata(J, -1, "af_fn");
     js_pop(J, 2);
-    void *af = talloc_new(NULL);
+    void *af = NULL;
-    int r = s_run_af_jsc(J, fn, af);
+    int r = s_run_af_jsc(J, fn, &af);
     talloc_free(af);
     if (r)
         js_throw(J);
 }
 // Safely run af_push_file.
-static int s_push_file(js_State *J, const char *fname, int limit, void *af)
+static int s_push_file(js_State *J, const char *fname, int limit, void **af)
 {
     if (js_try(J))
         return 1;
-    af_push_file(J, fname, limit, af);
+    *af = talloc_new(NULL);
+    af_push_file(J, fname, limit, *af);
     js_endtry(J);
     return 0;
 }
 // Called directly, push up to limit bytes of file fname (from builtin/os).
 static void push_file_content(js_State *J, const char *fname, int limit)
 {
-    void *af = talloc_new(NULL);
+    void *af = NULL;
-    int r = s_push_file(J, fname, limit, af);
+    int r = s_push_file(J, fname, limit, &af);
     talloc_free(af);
     if (r)
         js_throw(J);

js: fix tiny leaks if js_try throws(!)

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code