Bug 82246 (atexit)

Thanks.  Did you actually notice this in any real setting?  There are more invocations of atexit, I'll switch them to use destructors if they are supported.

I'm currently debugging why gmpc dumps core during exiting, and this is one of the atexit() calls I've found that I thought might be the cause. The patch wasn't sufficient though, so I'm currently looking for more...

Quoting man atexit:

   """Linux Notes
       Since glibc 2.2.3, atexit() (and on_exit(3)) can be used within a shared library to establish functions that are called when the shared library is unloaded."""


So, I think this is as safe as we care on glibc-based systems.

From:

  http://msdn.microsoft.com/en-ca/library/tze57ck3.aspx

   """The code in the atexit function should not contain any dependency on any DLL which could have already been unloaded when the atexit function is called."""

Which makes me believe that it's safe to use it in DLLs as long as it doesn't call other DLLs.  However, there's also _onexit:

  http://msdn.microsoft.com/en-ca/library/zk17ww08.aspx

that specifically says:

   """In the case when _onexit is called from within a DLL, routines registered with _onexit run on a DLL's unloading after DllMain is called with DLL_PROCESS_DETACH.

_onexit is a Microsoft extension. For ANSI portability, use atexit."""

Reading around the net:

  http://arstechnica.com/civis/viewtopic.php?f=20&t=438235

I get the impression that atexit() is also called during DLL unload just like _onexit().

It used to be an issue on bionic / Android, but seems to have been fixed in Nov 2012:

  https://code.google.com/p/android/issues/detail?id=6455

As such, I think this is working as intended.

Thanks for the report though.  Until someone comes with a legitimate system that this doesn't work, I'll leave it as is.  Only then I'll add conditionals for all the systems that we know this is safe to do.

FreeBSD does not promise this:

http://www.freebsd.org/cgi/man.cgi?query=atexit&apropos=0&sektion=0&manpath=FreeBSD+9.3-RELEASE&arch=default&format=html

Nor does NetBSD:

http://netbsd.gw.com/cgi-bin/man-cgi?atexit++NetBSD-current

Nor does OpenBSD:

http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/atexit.3?query=atexit

Ok, thanks.  I'll try a whitelist approach then.

commit 38fb30d7420a4b01f99cee31baa8c3990a1d1c5f
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Wed Aug 6 13:34:49 2014 -0400

    Use atexit() only if it's safe to call from shared library
    
    Apparently they are not (advertised as?) safe on BSD systems.
    We ignore the case of static libraries.
    
    Whitelisted on glibc, Android, and MSVC / mingw.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=82246

diff --git a/src/hb-private.hh b/src/hb-private.hh
index 5a4ca69..1b39e57 100644
--- a/src/hb-private.hh
+++ b/src/hb-private.hh
@@ -130,6 +130,31 @@
 #  define STRICT
 #endif
 
+#if HAVE_ATEXIT
+/* atexit() is only safe to be called from shared libraries on certain
+ * platforms.  Whitelist.
+ * https://bugs.freedesktop.org/show_bug.cgi?id=82246 */
+#  if defined(__linux) && defined(__GLIBC_PREREQ)
+#    if __GLIBC_PREREQ(2,3)
+/* From atexit() manpage, it's safe with glibc 2.2.3 on Linux. */
+#      define HB_USE_ATEXIT 1
+#    endif
+#  elif defined(_MSC_VER) || defined(__MINGW32__)
+/* For MSVC:
+ * http://msdn.microsoft.com/en-ca/library/tze57ck3.aspx
+ * http://msdn.microsoft.com/en-ca/library/zk17ww08.aspx
+ * mingw32 headers say atexit is safe to use in shared libraries.
+ */
+#    define HB_USE_ATEXIT 1
+#  elif defined(__ANDROID__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
+/* This was fixed in Android NKD r8 or r8b:
+ * https://code.google.com/p/android/issues/detail?id=6455
+ * which introduced GCC 4.6:
+ * https://developer.android.com/tools/sdk/ndk/index.html
+ */
+#    define HB_USE_ATEXIT 1
+#  endif
+#endif

Thank you, that is an improvement.
How about the leak on the platforms that don't have atexit support for unloading shared objects? Would you consider a variant of my patch for them, or at least for another set of whitelisted platforms?

Ok, I see what you mean.

Some background as to why I introduced atexit:

Inititally, I was using static C++ classes with constructors and destructors for this purpose.  However, when we were porting Chrome to use HarfBuzz-ng, the Chrome team didn't like it.  They are strictly against static constructors since those add directly to the app start time.

One alternative was to move them into function scope.  That delays the initialization to the first time the function is entered.  However, that would require linking libstdc++, which is something we want to avoid.

As such, I turned them into plain old static C data and implemented thread-safe initialization explicitly.  At that point (ie. avoiding C++), I found atexit() the easiest way to reclaim the memory portably.

Now, perhaps it's ok to use destructor attributes and they might be more portable, or perhaps we need both...  In that case might be easier to introduce an hb_onexit() polyfill.

I'll checkout simpleops to decide:

  http://cgit.freedesktop.org/~ranma42/simpleops/log/

Feel free to submit patch though, I'm a bit busy with other work right now.  Thanks!

Thanks for your comments.
I'm told by people whose expertise I trust that the destructor markup is very portable, but I can't confirm this myself.

Anyway, more to do here, leave this open for now.

Thanks.  "Very portable" is heartwarming but doesn't change the fact that it still needs whitelisting as it doesn't work with the same syntax on Windows.

Lets add a hb_atexit() that either simply is an alias for atexit() or to a function that fills in a static array that is then freed by an attribute/pragma'ed function.

Marking fixed as there's no actionable item left.