Don't neglect to forget self in close

Otherwise it leads to a double-free, once in `close` and another time in
`drop`. Curiously this was only a bug on Windows, and Unix code was
fine. Not sure how this happened.

Author: nagisa

src/lib.rs

@@ -199,6 +199,8 @@ impl Library {
     /// You only need to call this if you are interested in handling any errors that may arise when
     /// library is unloaded. Otherwise the implementation of `Drop` for `Library` will close the
     /// library and ignore the errors were they arise.
+    ///
+    /// The underlying data structures may still get leaked if an error does occur.
     pub fn close(self) -> Result<(), Error> {
         self.0.close()
     }

src/os/unix/mod.rs

@@ -301,6 +301,8 @@ impl Library {
     ///
     /// You only need to call this if you are interested in handling any errors that may arise when
     /// library is unloaded. Otherwise this will be done when `Library` is dropped.
+    ///
+    /// The underlying data structures may still get leaked if an error does occur.
     pub fn close(self) -> Result<(), crate::Error> {
         let result = with_dlerror(|desc| crate::Error::DlClose { desc }, || {
             if unsafe { dlclose(self.handle) } == 0 {
@@ -309,6 +311,9 @@ impl Library {
                 None
             }
         }).map_err(|e| e.unwrap_or(crate::Error::DlCloseUnknown));
+        // While the library is not free'd yet in case of an error, there is no reason to try
+        // dropping it again, because all that will do is try calling `FreeLibrary` again. only
+        // this time it would ignore the return result, which we already seen failing…
         std::mem::forget(self);
         result
     }

src/os/windows/mod.rs

@@ -217,14 +217,24 @@ impl Library {
     }
 
     /// Unload the library.
+    ///
+    /// You only need to call this if you are interested in handling any errors that may arise when
+    /// library is unloaded. Otherwise this will be done when `Library` is dropped.
+    ///
+    /// The underlying data structures may still get leaked if an error does occur.
     pub fn close(self) -> Result<(), crate::Error> {
-        with_get_last_error(|source| crate::Error::FreeLibrary { source }, || {
+        let result = with_get_last_error(|source| crate::Error::FreeLibrary { source }, || {
             if unsafe { libloaderapi::FreeLibrary(self.0) == 0 } {
                 None
             } else {
                 Some(())
             }
-        }).map_err(|e| e.unwrap_or(crate::Error::FreeLibraryUnknown))
+        }).map_err(|e| e.unwrap_or(crate::Error::FreeLibraryUnknown));
+        // While the library is not free'd yet in case of an error, there is no reason to try
+        // dropping it again, because all that will do is try calling `FreeLibrary` again. only
+        // this time it would ignore the return result, which we already seen failing…
+        std::mem::forget(self);
+        result
     }
 }
 

tests/functions.rs

@@ -145,6 +145,30 @@ fn test_static_ptr() {
     }
 }
 
+#[test]
+// Something about i686-pc-windows-gnu, makes dll initialization code call abort when it is loaded
+// and unloaded many times. So far it seems like an issue with mingw, not libloading, so ignoring
+// the target. Especially since it is very unlikely to be fixed given the state of support its
+// support.
+#[cfg(not(all(target_arch="x86", target_os="windows", target_env="gnu")))]
+fn manual_close_many_times() {
+    make_helpers();
+    let join_handles: Vec<_> = (0..16).map(|_| {
+        std::thread::spawn(|| unsafe {
+            for _ in 0..10000 {
+                let lib = Library::new(LIBPATH).expect("open library");
+                let _: Symbol<unsafe extern fn(u32) -> u32> =
+                    lib.get(b"test_identity_u32").expect("get fn");
+                lib.close().expect("close is successful");
+            }
+        })
+    }).collect();
+    for handle in join_handles {
+        handle.join().expect("thread should succeed");
+    }
+}
+
+
 #[cfg(unix)]
 #[test]
 fn library_this_get() {