[[ Bug 22010 ]] Fix memory leak when binding LCB foreign handlers

runrevmark · runrevmark · commit fc309f365474 · 2019-04-18T20:24:31.000+01:00
This patch fixes a memory leak which can occur when binding to LCB
foreign handlers. Previously, if a foreign handler was binding to
a C or Objective-C function and was present in an external code
library then a leak of the native code module handle wrapper could
occur.

This patch fixes the issue by holding any such handles in a per-module
array keyed by the name of the external native code library being loaded.
diff --git a/docs/notes/bugfix-22010.md b/docs/notes/bugfix-22010.md
@@ -0,0 +1 @@
+# Fix memory leak when binding some LCB foreign handlers
diff --git a/libscript/include/libscript/script.h b/libscript/include/libscript/script.h
@@ -255,6 +255,9 @@ void MCScriptSetModuleLicensed(MCScriptModuleRef self, bool p_licensed);
 // Gets the licensed state of a module
 bool MCScriptIsModuleLicensed(MCScriptModuleRef self);
 
+// Attempt to load the named library using any context provided by the module
+bool MCScriptLoadModuleLibrary(MCScriptModuleRef self, MCStringRef p_library, MCSLibraryRef& r_library);
+
 ////////////////////////////////////////////////////////////////////////////////
 
 // Create an instance of the given module. If the module is single-instance it
diff --git a/libscript/src/script-instance.cpp b/libscript/src/script-instance.cpp
@@ -805,28 +805,20 @@ __MCScriptResolveForeignFunctionBindingForC(MCScriptInstanceRef p_instance,
     {
         return false;
     }
-    
-    /* TODO: This leaks a module handle if library is not empty (builtin) */
+
     MCSLibraryRef t_module;
-    if (MCStringIsEmpty(*t_library))
+    if (!MCScriptLoadModuleLibrary(MCScriptGetModuleOfInstance(p_instance),
+                                   *t_library,
+                                   t_module))
     {
-        t_module = MCScriptGetLibrary();
-    }
-    else
-    {
-        if (!MCScriptLoadLibrary(MCScriptGetModuleOfInstance(p_instance),
-                                 *t_library,
-                                 t_module))
+        if (r_bound == nil)
         {
-            if (r_bound == nil)
-            {
-                return MCScriptThrowUnableToLoadForiegnLibraryError();
-            }
+            return MCScriptThrowUnableToLoadForiegnLibraryError();
+        }
         
-            *r_bound = false;
+        *r_bound = false;
         
-            return true;
-        }
+        return true;
     }
     
     /* Resolve the symbol from the module which we've loaded */
@@ -948,27 +940,19 @@ __MCScriptResolveForeignFunctionBindingForObjC(MCScriptInstanceRef p_instance,
         return false;
     }
     
-    /* TODO: This leaks a module handle if library is not empty (builtin) */
     MCSLibraryRef t_module;
-    if (MCStringIsEmpty(*t_library))
-    {
-        t_module = MCScriptGetLibrary();
-    }
-    else
+    if (!MCScriptLoadModuleLibrary(MCScriptGetModuleOfInstance(p_instance),
+                                   *t_library,
+                                   t_module))
     {
-        if (!MCScriptLoadLibrary(MCScriptGetModuleOfInstance(p_instance),
-                                 *t_library,
-                                 t_module))
+        if (r_bound == nil)
         {
-            if (r_bound == nil)
-            {
-                return MCScriptThrowUnableToLoadForiegnLibraryError();
-            }
+            return MCScriptThrowUnableToLoadForiegnLibraryError();
+        }
         
-            *r_bound = false;
+        *r_bound = false;
         
-            return true;
-        }
+        return true;
     }
     
     MCScriptThreadAffinity t_thread_affinity;
diff --git a/libscript/src/script-module.cpp b/libscript/src/script-module.cpp
@@ -284,6 +284,13 @@ void MCScriptDestroyModule(MCScriptModuleRef self)
                 break;
             }
     
+    // Free any loaded libraries
+    if (self->libraries != nullptr)
+    {
+        MCValueRelease(self->libraries);
+    }
+    
+    // Free the compiled module representation
     MCPickleRelease(kMCScriptModulePickleInfo, self);
 }
 
@@ -909,6 +916,74 @@ bool MCScriptEnsureModuleIsUsable(MCScriptModuleRef self)
 	return false;
 }
 
+bool
+MCScriptLoadModuleLibrary(MCScriptModuleRef self,
+                          MCStringRef p_library_name_string,
+                          MCSLibraryRef& r_library)
+{
+    /* If the string is empty, then use the library containing libscript (i.e.
+     * 'builtin'). */
+    if (MCStringIsEmpty(p_library_name_string))
+    {
+        r_library = MCScriptGetLibrary();
+        return true;
+    }
+    
+    /* Create a nameref for the library so we can lookup up in the module's
+     * library list. */
+    MCNewAutoNameRef t_library_name;
+    if (!MCNameCreate(p_library_name_string,
+                      &t_library_name))
+    {
+        return false;
+    }
+    
+    /* If there is a libraries array, then check whether the library has been
+     * loaded before, and if so use that library. Otherwise create the libraries
+     * array. */
+    if (self->libraries != nullptr)
+    {
+        if (MCArrayFetchValue(self->libraries,
+                              true,
+                              *t_library_name,
+                              (MCValueRef&)r_library))
+        {
+            return true;
+        }
+    }
+    else
+    {
+        if (!MCArrayCreateMutable(self->libraries))
+        {
+            return false;
+        }
+    }
+    
+    /* Attempt to load the library */
+    MCSAutoLibraryRef t_library;
+    if (!MCScriptLoadLibrary(self,
+                             p_library_name_string,
+                             &t_library))
+    {
+        return false;
+    }
+    
+    /* Store the library in the libraries array for the module. */
+    if (!MCArrayStoreValue(self->libraries,
+                           true,
+                           *t_library_name,
+                           *t_library))
+    {
+        return false;
+    }
+    
+    /* Return the library (unretained - as the module holds a reference through
+     * its array). */
+    r_library = *t_library;
+    
+    return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 static bool
diff --git a/libscript/src/script-private.h b/libscript/src/script-private.h
@@ -512,6 +512,11 @@ struct MCScriptModule: public MCScriptObject
     
     // This is the ordinal mapping array (if any) -- not pickled
     void **builtins;
+    
+    // This is a map from library name used in the module to MCSLibraryRef
+    // all foreign handler definitions which have the same library string share
+    // a single MCSLibraryRef -- not pickled
+    MCArrayRef libraries;
 };
 
 bool MCScriptWriteRawModule(MCStreamRef stream, MCScriptModule *module);

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+# Fix memory leak when binding some LCB foreign handlers`